Query param changes during navigation breaks FlowRouter

[arggh]

A call to FlowRouter.setQueryParams({ foo: 'bar' }), at approximately the same time as a navigation happens, breaks the FlowRouter completely - rendering the app useless.

See reproduction with instructions included in the repro app itself:

https://github.com/arggh/flow-router-waiton-issue

Just clone the repro, run npm install and meteor.

Situation where this occurred and explanation what happens:

1. There is a link inside a div, that takes us to another page.
2. We also have a click event listener on the link's parent div, which has a handler that will call FlowRouter.setQueryParams`.
3. User clicks on the link
4. Click listener will cause a query parameter change to be triggered
5. waitOn will get called on the current route
6. Navigation takes place and FlowRouter picks up route change
7. waitOn will get called on the new route
8. endWaiting will get called twice on the new route
9. action will also get called twice on the new route, once before the dynamic import promise has been resolved in waitOn, thus breaking the app.
10. Any further attempts at navigating will not work

• Version of flow-router-extra you're experiencing this issue 3.6.3
• Version of Meteor you're experiencing this issue 1.8.1
• Browser name and its version All of them
• If you're getting an error or exception, please provide its full stack-trace as plain-text or screenshot

meteor.js?hash=857dafb4b9dff17e29ed8498a22ea5b1a3d6b41d:1059 Exception from Tracker recompute function:
meteor.js?hash=857dafb4b9dff17e29ed8498a22ea5b1a3d6b41d:1059 Error: Can't render undefined
    at checkRenderContent (blaze.js?hash=51f4a3bdae106610ee48d8eff291f3628713d847:2285)
    at contentAsFunc (blaze.js?hash=51f4a3bdae106610ee48d8eff291f3628713d847:2328)
    at Object.Blaze.renderWithData (blaze.js?hash=51f4a3bdae106610ee48d8eff291f3628713d847:2401)
    at BlazeRenderer.materialize (renderer.js:341)
    at BlazeRenderer._load (renderer.js:258)
    at BlazeRenderer.proceed (renderer.js:198)
    at BlazeRenderer.startQueue (renderer.js:98)
    at BlazeRenderer.render (renderer.js:90)
    at Route.action [as _action] (main.js:34)
    at Route.callAction (route.js:304)

[dr-dimitru]

@arggh thank you for reporting this one

I see on your screenshot before exception it throws can't render undefined, any idea why?

[arggh]

I see on your screenshot before exception it throws can't render undefined, any idea why?

Because the template it's trying to render hasn't been imported yet, due to the glitch described in this issue.

If you modify the reproduction and remove the layout template from this.render calls, you will get a more meaningful error message. Using the layout-template (which contains {{> yield}}) will cause this obscure error message to be shown instead (can't render undefined).

edit: Maybe the fact that using a layout template with {{> yield }} obscures the error message is worth another issue?

[dr-dimitru]

@arggh I cloned the repo and running locally, can't really reproduce it.
No exceptions or errors on my end. Going to try more different browsers

[dr-dimitru]

@arggh got production in [email protected] Safari and earlier versions of Chrome are fine

[arggh]

It's pretty obvious but I'll still mention this: if you access route /b directly first, the template gets dynamically imported and the error no longer appears. So to reproduce, you should refresh the browser at /, then navigate to template A, then template B via clicking the link "Navigate to A".

I can reproduce each and every time on Chrome version 74.0.3729.157, Safari version 12.1 (12607.1.40.1.5) and Firefox 67.0.

[arggh]

Here are screen captures from Safari & Firefox:

Safari 12.1:

Firefox 67:

(the screen capture in first post was on Chrome 74)

[dr-dimitru]

@arggh FYI I'm working on fix, it's caused by our logic, as we assume you'd like to abort all pending subscriptions and Promises when you navigating to another route, and calling setQueryParam invokes route navigation logic.

1. Is there a real use case behind this app sample?
2. Can you move queryString to link or .go() method?

[arggh]

1. Is there a real use case behind this app sample?

I would argue, yes. Say you have a modal opened, for example a message from another user. This is persisted in the url with a query parameter, ?msg=xk53. A common practice is to close a modal once a user clicks anywhere outside it. Once the user has read the message, he/she clicks a navigational link in the header, simultaneously triggering a) closing the modal, thus removing the ?msg=xk53 param from query string and b) navigating to new route.

Only now the app is just broken. This was also quite similar to the situation where I bumped into this issue.

2. Can you move queryString to link or .go() method?

I'm sure there's almost always a way to circumvent this situation from happening, but I think that doesn't mean a fix shouldn't be attempted?

[dr-dimitru]

Hello @arggh ,

Very sorry for the late reply, as well as this may not fix the issue you are experiencing.
Please try to upgrade to the latest v3.7.0 release, let me know if it would fix your issue.

[dr-dimitru]

@arggh friendly ping 🛸

[arggh]

Sorry for the eerie silence. I was, or am currently, a bit overrun by life at the moment.

I took the newest 3.7.2 for a spin and tried my repro, with this as a result:

WAIT ON A
main.js:26 WAIT ON B
main.js:30 END WAITING B
main.js:33 RENDER B undefined
meteor.js:1059 Exception from Tracker recompute function:
meteor.js:1059 Error: No such template: b [404]
    at BlazeRenderer.proceed (renderer.js:172)
    at BlazeRenderer.startQueue (renderer.js:99)
    at BlazeRenderer.render (renderer.js:91)
    at Route.action [as _action] (main.js:34)
    at Route.callAction (route.js:313)
    at router.js:517
    at Object.Tracker.nonreactive (tracker.js:603)
    at router.js:502
    at Computation._compute (tracker.js:308)
    at Computation._recompute (tracker.js:324)
main.js:30 END WAITING B
main.js:33 RENDER B Blaze.Template {viewName: "Template.b", __helpers: HelperMap, __eventMaps: Array(0), _callbacks: {…}, renderFunction: ƒ}
b.js:5 B was creaated.

This would indicate that the actual bug is still unfixed, though the error message is improved apparently?

[dr-dimitru]

Hello @arggh ,

I ran tests against your reproduction app, although error message is displayed, there is no exception and app not getting broken (users still can navigate, etc.)

I'll keep looking for a solution to fix it.

[dr-dimitru]

@arggh finally found a solution! Actually it has positive impact on our test-suite.
Please update to v3.7.3

Feel free to reopen it in case if the issue is still persists on your end.

[dr-dimitru]

@arggh friendly ping. Is it fixed on your end?

[arggh]

Sorry, but I still get the error when I run the reproduction with 3.7.3:

WAIT ON A
main.js:15 END WAITING A
main.js:18 RENDER A Blaze.Template {viewName: "Template.a", __helpers: HelperMap, __eventMaps: Array(1), _callbacks: {…}, renderFunction: ƒ}
main.js:11 WAIT ON A
main.js:15 END WAITING A
main.js:18 RENDER A Blaze.Template {viewName: "Template.a", __helpers: HelperMap, __eventMaps: Array(1), _callbacks: {…}, renderFunction: ƒ}
main.js:11 WAIT ON A
main.js:26 WAIT ON B
main.js:30 END WAITING B
main.js:33 RENDER B undefined
meteor.js?hash=857dafb4b9dff17e29ed8498a22ea5b1a3d6b41d:1059 Exception from Tracker recompute function:
meteor.js?hash=857dafb4b9dff17e29ed8498a22ea5b1a3d6b41d:1059 Error: No such template: b [404]
    at BlazeRenderer.proceed (renderer.js:172)
    at BlazeRenderer.startQueue (renderer.js:99)
    at BlazeRenderer.render (renderer.js:91)
    at Route.action [as _action] (main.js:34)
    at Route.callAction (route.js:311)
    at router.js:517
    at Object.Tracker.nonreactive (tracker.js:603)
    at router.js:502
    at Computation._compute (tracker.js:308)
    at Computation._recompute (tracker.js:324)
main.js:30 END WAITING B
main.js:33 RENDER B Blaze.Template {viewName: "Template.b", __helpers: HelperMap, __eventMaps: Array(0), _callbacks: {…}, renderFunction: ƒ}
b.js:5 B was creaated.

[dr-dimitru]

@arggh opening this issue due to your report

//cc @thearabbit

[dr-dimitru]

Hope this one related and will be fixed by #73 and #74
//cc @jankapunkt