[BUGFIX beta] Avoid run.next in app.visit resolve handler.
#14591
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
/cc @chancancode since the original comment was added in #12394. |
kratiahuja
reviewed
Nov 8, 2016
| // a scheduled revalidation (into the `render` queue) in the Renderer. | ||
| // | ||
| // This uses `run.schedule('afterRender', ....)` to resolve after that rendering has completed. | ||
| run.schedule('afterRender', null, resolve, this); |
There was a problem hiding this comment.
Is this fine if the app is being visited in routing-only mode on server side?
There was a problem hiding this comment.
Interesting! I hadn't thought of that when reviewing this, but you are correct when shouldRender is false we can just return this; right away I believe.
There was a problem hiding this comment.
Yup basically stop the execution after the actions queue is flushed.
rwjblue
force-pushed
the
cleanup-visit-comments
branch
from
32a7966 to
6630237
Compare
The prior comment has been pointed to by a few different people in the
FastBoot team meetings so I decided to dig into exactly what the TODO
was talking about. Specifically, why do we need a `run.next` before
resolving the visit promise?
The answer is basically that the `router.handleURL` transition promise
being resolved does not actually guarantee that rendering is completed.
Since the renderer schedules the actual revalidation and rendering into
the `render` queue, we must wait for that to complete before resolving
the promise.
This commit explains that in the inline comments, and changes the
implementation to use `run.schedule('afterRender', ....)` instead of
`run.next` (since resolving "after rendering is complete" is what we
actually care about). It also avoids waiting at all when `shouldRender:
false` is passed into the `visit` API (since there is nothing to do in
that case).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The prior comment has been pointed to by a few different people in the FastBoot team meetings so I decided to dig into exactly what the TODO was talking about. Specifically, why do we need a
run.nextbefore resolving the visit promise?The answer is basically that the
router.handleURLtransition promise being resolved does not actually guarantee that rendering is completed. Since the renderer schedules the actual revalidation and rendering into therenderqueue, we must wait for that to complete before resolving the promise.This commit explains that in the inline comments, and changes the implementation to use
run.schedule('afterRender', ....)instead ofrun.next(since resolving "after rendering is complete" is what we actually care about).