router: delay mounting RouterRoot until the first route is resolved (fixes #2621)#3030
Conversation
dead-claudia
left a comment
dead-claudia
left a comment
There was a problem hiding this comment.
Could you provide tests for the new behavior?
The following tests were added. - Existing content is cleared after the route is resolved. - Errors are not caught by m.mount when the route component is mounted for the first time.
|
@dead-claudia |
The following tests were added. - Existing content is cleared after the route is resolved. - Errors are not caught by m.mount when the route component is mounted for the first time.
kfule
force-pushed
the
delay-router-mount
branch
from
2369c9b to
b27020d
Compare
dead-claudia
left a comment
dead-claudia
left a comment
There was a problem hiding this comment.
Could you also add tests to ensure it shakes out with render? I would like tests for the same stuff, but I also want to know the precise tree state to expect during the first call to m.render.
I assume the tree would be cleared before it's called, as that's how it's currently architected, but I want this enshrined in tests so we know what specifically is supposed to be happening in that edge case.
|
@dead-claudia If there are any other test cases you'd like me to add, please let me know with some details. That would be very helpful🙂 |
|
@kfule I'm okay with those tests, but I was really looking in particular for two things:
|
|
@dead-claudia
I think these cases could probably be indirectly confirmed in previous tests, but I didn't immediately find any test cases that directly corresponded to them. |
…fixes MithrilJS#2621) This commit delays the initial mounting of the router component until after the route has been resolved. The actual component is initially mounted, which fixes MithrilJS#2621. In addition, since the root DOM is not manipulated until the actual mounting, it makes easy to display something like "loading..." when using `onmatch` (cf. MithrilJS#2490). Also, this commit adds cleanup code to RouterRoot's `onremove`. This keeps all existing tests pass.
This reduces the bundle size slightly.
The following tests were added. - Existing content is cleared after the route is resolved. - Errors are not caught by m.mount when the route component is mounted for the first time.
In addition, text has been added to make the changes to the tree clearer.
…resolver's render method or view method
…ot is cleared synchronously)
dead-claudia
force-pushed
the
delay-router-mount
branch
from
f12ffe4 to
c65f4af
Compare
2 participants
Description
This PR delays the initial mounting of the router component until after the route has been resolved.
Since the component that displays nothing is no longer mounted first, the following improvements are made.
m.mountwhen the actual component is first rendered, and Errors are not forwarded to window.onerror #2621 is fixed.onmatch(cf. m.render inside route onmatch causes infinite loop #2490).In addition, cleanup code has been added to RouterRoot's
onremoveto pass the test.Motivation and Context
fixes #2621, and enables "loading..." like #2490
How Has This Been Tested?
npm run testwith new testsI have also confirmed that replacing the bundle improved the behavior of the following flems.
window.onerror.Types of changes
Checklist