[BUGFIX lts] Fix tag cycles in query parameters #19138
Merged
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
pzuraq
force-pushed
the
bugfix/fix-tag-cycles-in-query-params
branch
from
f782179
to
2a73dd3
Compare
|
@pzuraq - Can you review the open issues RE: QP + cycles, and cross link the ones this fixes? |
chriskrycho
reviewed
Sep 14, 2020
|
🙌 #18981 |
|
We should probably add a QP test for this? |
Yes, absolutely. That was why I was hoping we could cross link the related issues, and create test cases for them... |
|
I think I have a small repro in twiddle, but maybe not small enough for a test case |
pzuraq
force-pushed
the
bugfix/fix-tag-cycles-in-query-params
branch
from
2a73dd3
to
6659046
Compare
|
Note that there was already a test case, which was passing mistakenly due to the ALLOW_CYCLES logic being faulty. Fixing that logic caused the test to fail, and then the rest of the PR fixed it. Noted this in the original PR description. @rwjblue did link me to a twiddle with an additional failure that somehow was caused by using the |
pzuraq
force-pushed
the
bugfix/fix-tag-cycles-in-query-params
branch
from
6659046
to
aa7fa88
Compare
pzuraq
force-pushed
the
bugfix/fix-tag-cycles-in-query-params
branch
from
aa7fa88
to
1508fce
Compare
Currently, we wrap query parameters with @dependentKeyCompat manually to ensure they work with the legacy router setup, which watches QPs using observers. When a QP value is labeled as @Tracked, we don't need to replace it with @dependentKeyCompat, but since it's just a plain native getter (from the decorator) we do anyways. This results in a tag cycle being created, which can result in a negative performance impact, as every render pass will be invalidated by the cycle and require a subsequent full revalidation. This bug would have been caught by our ALLOW_CYCLES logic, but that logic was faulty - it was allowing too many cycles, anything that was accessed via get. It should only have allowed cycles from computeds themselves. This PR fixes the bug with ALLOW_CYCLES, and the subsequent bug with @Tracked by only wrapping with @dependentKeyCompat if the value is not @Tracked. It also adds an assertion to prevent users from using @dependentKeyCompat with @computed or @Tracked. Verified that the test failed after fixing ALLOW_CYCLES, and passed again after the fix.
rwjblue
force-pushed
the
bugfix/fix-tag-cycles-in-query-params
branch
from
1508fce
to
57a2044
Compare
rwjblue
approved these changes
Sep 30, 2020
There was a problem hiding this comment.
Rebased on top of all the prettier/eslint changes that just landed on master, and fixed up the error message issue that @chriskrycho pointed out.
Should be good to land once CI is happy...
pzuraq
pushed a commit
that referenced
this pull request
Oct 1, 2020
The bugfix for disallowed cycles in tracked props in #19138 attempted to also narrow the number of cycles that are allowed in general. Cycles should only be allowed for computed property deps, for legacy support reasons. The logic to allow cycles for these tags in particular was mistakenly added to `setup`, which runs on the _prototype_ of the class. This meant that _instance_ computed props were not allowed to have cycles, and this was causing failures in the ecosystem. Added a test that failed and is fixed with this change. I also attempted to create a cycle with `@alias` since it uses a different implementation, but I wasn't able to create one which didn't result in a Maximum Callstack style recursion error, so I think it's just not possible at all anyways, since `@alias` is eager always and never caches.
pzuraq
pushed a commit
that referenced
this pull request
Oct 1, 2020
The bugfix for disallowed cycles in tracked props in #19138 attempted to also narrow the number of cycles that are allowed in general. Cycles should only be allowed for computed property deps, for legacy support reasons. The logic to allow cycles for these tags in particular was mistakenly added to `setup`, which runs on the _prototype_ of the class. This meant that _instance_ computed props were not allowed to have cycles, and this was causing failures in the ecosystem. Added a test that failed and is fixed with this change. I also attempted to create a cycle with `@alias` since it uses a different implementation, but I wasn't able to create one which didn't result in a Maximum Callstack style recursion error, so I think it's just not possible at all anyways, since `@alias` is eager always and never caches.
|
Does this fix the performance issue that was present when some query params were used alongside tracked properties? |
rwjblue
pushed a commit
that referenced
this pull request
Nov 10, 2020
The bugfix for disallowed cycles in tracked props in #19138 attempted to also narrow the number of cycles that are allowed in general. Cycles should only be allowed for computed property deps, for legacy support reasons. The logic to allow cycles for these tags in particular was mistakenly added to `setup`, which runs on the _prototype_ of the class. This meant that _instance_ computed props were not allowed to have cycles, and this was causing failures in the ecosystem. Added a test that failed and is fixed with this change. I also attempted to create a cycle with `@alias` since it uses a different implementation, but I wasn't able to create one which didn't result in a Maximum Callstack style recursion error, so I think it's just not possible at all anyways, since `@alias` is eager always and never caches. (cherry picked from commit b612d47)
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.
Currently, we wrap query parameters with
@dependentKeyCompatmanuallyto ensure they work with the legacy router setup, which watches QPs
using observers. When a QP value is labeled as
@tracked, we don't needto replace it with
@dependentKeyCompat, but since it's just a plainnative getter (from the decorator) we do anyways. This results in a tag
cycle being created, which can result in a negative performance impact,
as every render pass will be invalidated by the cycle and require a
subsequent full revalidation.
This bug would have been caught by our ALLOW_CYCLES logic, but that
logic was faulty - it was allowing too many cycles, anything that was
accessed via
get. It should only have allowed cycles from computedsthemselves.
This PR fixes the bug with ALLOW_CYCLES, and the subsequent bug with
@trackedby only wrapping with@dependentKeyCompatif the value isnot
@tracked. It also adds an assertion to prevent users from using@dependentKeyCompatwith@computedor@tracked.Verified that the test failed after fixing ALLOW_CYCLES, and passed
again after the fix.