[BUGFIX] Ensure QP definitions interop with tracked props #18358
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
rwjblue
previously requested changes
Sep 6, 2019
This PR ensures that QPs can autotrack getters/setters and tracked props. It includes a couple of manual flushes of async observers in order to support legacy sync behaviors of QPs.
pzuraq
force-pushed
the
bugfix/ensure-query-params-work-with-tracked-props
branch
from
529feee to
7f29c04
Compare
chancancode
approved these changes
Sep 12, 2019
pzuraq
dismissed
rwjblue’s stale review
Dismissing because the PR has been updated, and requested changes are no longer relevant
pzuraq
deleted the
bugfix/ensure-query-params-work-with-tracked-props
branch
rwjblue
reviewed
Sep 14, 2019
| addObserver(key, target, method) { | ||
| addObserver(this, key, target, method); | ||
| addObserver(key, target, method, async) { | ||
| addObserver(this, key, target, method, async); |
There was a problem hiding this comment.
We really need a test for this (unrelated to query params)
| // Some QPs have been updated, and those changes need to be propogated | ||
| // immediately. Eventually, we should work on making this async somehow. | ||
| if (EMBER_METAL_TRACKED_PROPERTIES && qpUpdated === true) { | ||
| flushAsyncObservers(false); |
There was a problem hiding this comment.
It is not clear to me at all why we need to sync flush here. Can you make an issue for us to track removing this (likely evaluating the specific failing test that caused it to be made sync)?
There was a problem hiding this comment.
For reference, the failing tests were:
<LinkTo /> component with query params (routing): the <LinkTo /> component applies activeClass when query params are not changed
{{link-to}} component with query params (routing): the {{link-to}} applies activeClass when query params are not changed
| removeObserver(key, target, method) { | ||
| removeObserver(this, key, target, method); | ||
| removeObserver(key, target, method, async) { | ||
| removeObserver(this, key, target, method, async); |
There was a problem hiding this comment.
Needs a specific test, for users of observable (not related to query params).
wycats
pushed a commit
that referenced
this pull request
Sep 17, 2019
[BUGFIX] Ensure QP definitions interop with tracked props (cherry picked from commit 0efdae9)
wycats
pushed a commit
that referenced
this pull request
Sep 17, 2019
[BUGFIX] Ensure QP definitions interop with tracked props (cherry picked from commit 0efdae9)
rwjblue
added a commit
that referenced
this pull request
Sep 17, 2019
[BUGFIX] Ensure QP definitions interop with tracked props (#18358)
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.
This PR ensures QPs can be defined with native getters/setters or tracked properties. QP observers have been updated to always fire asynchronously, except in a few cases in the router's internals where we flush manually because we need to propagate changes immediately.