onRun hook is not fired EVER AGAIN when a hot code push happens
Closed this issue · 23 comments
The onRun hook is supposed to be fired when the route is ran, but not when the route is reran because of a computation invalidation. The doc states clearly that the onRun callback will not be ran when a hot code push reloads the page.
However, as soon as your app reloads because of a hotcode push, the onRun hook is never called again! Not even when you navigate to new routes. As soon as a route is activated, you expect this route to run the onRun callback (once), whether a hot code push has happened or not during your app runtime, no?
This can be a problem for a lot of people tracking their analytics on an onRun hook (a suggested in the docs). As soon as you deploy, all connected users will not trigger any analytics until they refresh.
When you add crucial functionality in the onRun callback, your app can break after deployment for connected users.
Also, during development, this issue is triggered after every file you save.
I added a very simple app with only iron-router as non-core package to be able to reproduce the issue:
https://github.com/Sewdn/iron-router-onrun
Thanks for looking into this!
Kr
I can confirm I am also seeing this
ok, that's what it is? I thought it was my mistake. guess this type of thing is hard to write tests for...
http://stackoverflow.com/questions/28728705/ironrouter-onrun-doesnt-fire
Router.map ->
@route 'slots/tags',
path: "/slots/:tags"
data: ->
console.log("IR:data")
blob = {
params: @params
tags: @params.tags
}
window.blob = blob
return blob
# wtf it doesnt seem to run?
onRun: ->
console.log("IR:onRun")
action: ->
console.log("IR:action")
createSlotsGame(@)
@render 'slots'
+1
Is there a temporary solution for this issue?
This is probably related to #1130.
This is by design. onRun is intended to run once, when the route first
runs. Use the action function or the onBeforeAction hook if you want
indefinite reactivity.
On Tue, Mar 3, 2015 at 10:06 AM, Paulo Borges notifications@github.com
wrote:
This is probably related to #1130
#1130.—
Reply to this email directly or view it on GitHub
#1219 (comment)
.
Please check this reproduction: https://github.com/pauloborges/iron-router-onrun-bug
You'll be able to see that on both routes, /test and /test2, the onRun method is being called. Now do this test: open /test and do a hot code push by, for example, changing Test's template. If you hit back and navigate to /test2 by clicking the second button, its onRun method won't be called.
Is this expected? I guess it's not. As @Sewdn said, if one hot code push happens, it will break all route's onRun methods.
@cmather: What Paul, I, and others are seeing is not that onRun doesn't run immediately after a hot code reload, but rather that onRun no longer executes for any future routes after a hot code reload.
e.g.
- navigate to route 1 -> onRun runs
- navigate away and then back to route 1 -> onRun runs
- hotcode reload -> onRun doesn't run (this is fine)
- navigate away and then back to route 1 -> (onRun doesn't run (this is the problem))
this may be the intended behavior but then onRun is probably not useful for analytics in the most common scenario (e.g. logging every time a user visits a route regardless if they have in the past) and a method for running code every time a route is hit would be hyper useful
Oh I see. That sounds buggish. Have you guys identified where the hot code stuff happens to take a crack at it?
On Mar 3, 2015, at 10:49 AM, funkyeah notifications@github.com wrote:
@cmather: What Paul, I, and others are seeing is not that onRun doesn't run immediately after a hot code reload, but rather that onRun no longer executes for any future routes after a hot code reload.
e.g.
navigate to route 1 -> onRun runs
navigate away and then back to route 1 -> onRun runs
hotcode reload -> onRun doesn't run (this is fine)
navigate away and then back to route 1 -> (onRun doesn't run (this is the problem))
—
Reply to this email directly or view it on GitHub.
i realized in my own code i have an onRun in the main controller. so this could be overriding/overwriting anything defined at a route level. i assumed it would be adding to the list of onRuns, kind of like how Meteor.onStartup works.
Apparently _hasJustReloaded never gets a falsey value after the first hot code push, so this condition is never true:
function onRun (req, res, next) {
if (this._computation.firstRun && !RouteController._hasJustReloaded) { //here
RouteController._hasJustReloaded = false;
if (onRunStack.length > 0) {
onRunStack.dispatch(req.url, this, next);
} else {
next();
}
} else {
next();
}
}
Source:
iron-router/lib/route_controller_client.js
Lines 197 to 208 in 17b3084
The other place where the variable is written is below, but I don't know how Meteor._reload works.
if (Meteor._reload) {
// just register the fact that a migration _has_ happened
Meteor._reload.onMigrate('iron-router', function() { return [true, true]});
// then when we come back up, check if it it's set
var data = Meteor._reload.migrationData('iron-router');
RouteController._hasJustReloaded = data;
}
Source:
iron-router/lib/route_controller_client.js
Lines 370 to 377 in 17b3084
@pauloborges, yep that's the place. And it looks like there might be some issue between _hasJustReloaded being set on the RouteController vs. the instance. But I'd have to spend time reasoning through it.
For @pauloborges or anyone else who wants to take a stab at fixing this: If you want to take 28 minutes to learn about hot code reload you can watch this class: https://www.eventedmind.com/classes/hot-code-push-with-reload-and-autoupdate. Email me and I'll give you a coupon where you can watch for free :).
@cmather I was having problems due to that bug with onRun not working after hot code pushes. I tried a small fix, it works for me... I created a test repo if you want to try it.
The value from RouteController._hasJustReloaded is never used anywhere else but for the onRun function, so it shouldn't cause any other problem.
The change is that simple:
function onRun (req, res, next) {
if (this._computation.firstRun && !RouteController._hasJustReloaded) {
RouteController._hasJustReloaded = false;
if (onRunStack.length > 0) {
onRunStack.dispatch(req.url, this, next);
} else {
next();
}
} else {
next();
}
},to:
function onRun (req, res, next) {
if (this._computation.firstRun && !RouteController._hasJustReloaded) {
if (onRunStack.length > 0) {
onRunStack.dispatch(req.url, this, next);
} else {
next();
}
} else {
next();
}
RouteController._hasJustReloaded = false;
},EDIT: Just created a pull request, if you think the changes looks good.
Oh, and I also updated the Meteor._reload code, which is now deprecated (Meteor reload package):
if (Package.reload) {
// just register the fact that a migration _has_ happened
Package.reload.Reload._onMigrate('iron-router', function() { return [true, true] });
// then when we come back up, check if it is set
var data = Package.reload.Reload._migrationData('iron-router');
RouteController._hasJustReloaded = data;
}EDIT2: I didn't include it but Meteor uses a weak dependency in packages using reload. Maybe it would be good to do the same (see example):
api.use('reload', 'client', {weak: true});+1
Yeah I'll add some time this week.
Thanks a lot!
+1
could anyone else be added to the IR team that has more time for these kind of things?
I guess with the state of meteor testing it makes it really hard to merge fixes for OSS projects, and if MDG don't want to maintain a router in core, we're down to 4 month wait times for PRs to be merged into IR.
+1 I don't like the idea of using a router which is being maintained this bad
:/ waiting!
Is this bug resolved? How's it going? I got the same issue.
270 issues, last commit a month ago (update readme), master not up to date with devel... I guess the author is busy, but it would be nice if someone could pick it up...
I'm not using it in my current projects, but people may need it in theirs.
PS: Look at the combo Session variables, dynamic template inclusion & template subscriptions. It might suit your needs, it works even better for mine...
it appears to have been fixed in march