flow-router icon indicating copy to clipboard operation
flow-router copied to clipboard

Query param changes during navigation breaks FlowRouter

Open arggh opened this issue 6 years ago • 17 comments

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.

May-22-2019 12-29-31

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)

arggh avatar May 22 '19 09:05 arggh

@arggh thank you for reporting this one

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

dr-dimitru avatar May 22 '19 09:05 dr-dimitru

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?

arggh avatar May 22 '19 10:05 arggh

@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 avatar May 22 '19 10:05 dr-dimitru

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

dr-dimitru avatar May 22 '19 11:05 dr-dimitru

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 avatar May 22 '19 11:05 arggh

Here are screen captures from Safari & Firefox:

Safari 12.1:

safari

Firefox 67:

firefox

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

arggh avatar May 23 '19 11:05 arggh

@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?

dr-dimitru avatar May 24 '19 16:05 dr-dimitru

  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.

  1. 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?

arggh avatar May 26 '19 21:05 arggh

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 avatar Sep 09 '19 14:09 dr-dimitru

@arggh friendly ping 🛸

dr-dimitru avatar Oct 03 '19 16:10 dr-dimitru

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?

arggh avatar Oct 07 '19 09:10 arggh

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 avatar Oct 12 '19 12:10 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 avatar Oct 12 '19 13:10 dr-dimitru

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

dr-dimitru avatar Nov 05 '19 20:11 dr-dimitru

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.

arggh avatar Nov 06 '19 15:11 arggh

@arggh opening this issue due to your report

//cc @thearabbit

dr-dimitru avatar Nov 27 '19 20:11 dr-dimitru

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

dr-dimitru avatar Mar 11 '20 03:03 dr-dimitru