[BUGFIX] Adds ability for computed props to depend on args #18217
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/add-unknown-property-tag-to-args-proxy
branch
3 times, most recently
from
c8d70bd
to
7ba847d
Compare
pzuraq
changed the title
pzuraq
force-pushed
the
bugfix/add-unknown-property-tag-to-args-proxy
branch
from
7ba847d
to
c09abf8
Compare
rwjblue
reviewed
Jul 25, 2019
packages/@ember/-internals/glimmer/lib/component-managers/custom.ts
Outdated
Show resolved
Hide resolved
pzuraq
force-pushed
the
bugfix/add-unknown-property-tag-to-args-proxy
branch
2 times, most recently
from
26b5b4b
to
31dd0fa
Compare
rwjblue
reviewed
Jul 30, 2019
packages/@ember/-internals/glimmer/tests/integration/components/tracked-test.js
Outdated
Show resolved
Hide resolved
| this.assertText('hello!'); | ||
| } | ||
|
|
||
| '@test named args are enumerable'() { |
There was a problem hiding this comment.
Can you add another test for using {{#each-in this.args as |arg value|}}?
There was a problem hiding this comment.
Good thing we added that, turns out it is necessary to support symbols in the proxy. I decided to make the guard in the get trap a bit more generic to handle that.
This PR adds the ability for computed properties and observers to watch `args` on custom components. Currently this does not work because the `args` object is not a standard object, but a proxy that forwards property access to the `CapturedNamedArgs` instance for the component, and consumes its tag for autotracking. CPs currently ignore autotracking, and have to depend on `args` manually via the `chainTags` system. In the first attempt to support this we used the `UNKNOWN_PROPERTY_TAG` API, but that actually doesn't work for this case because these _aren't_ unknown properties, they do "exist" on the proxy and should be enumerable. Rather than adding another general API to the `tagForProperty` function, I opted to add some special logic to the `chainTags` crawler, similar to what we do for `@each`. When the chain is looking up `args` as its segment, it'll also check to see if the value is an args proxy, and if so it'll use the (saved) CapturedNamedArgs directly. This check should be low overhead since it's a string comparison, and only applies to CPs and observers rather than _all_ code (compared to adding it to `tagForProperty`, which would affect new code as well).
pzuraq
force-pushed
the
bugfix/add-unknown-property-tag-to-args-proxy
branch
from
31dd0fa
to
fd5cff6
Compare
rwjblue
approved these changes
Jul 30, 2019
delete-merged-branch
bot
deleted the
bugfix/add-unknown-property-tag-to-args-proxy
branch
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 adds the ability for computed properties and observers to watch
argson custom components. Currently this does not work because theargsobject is not a standard object, but a proxy that forwardsproperty access to the
CapturedNamedArgsinstance for the component,and consumes its tag for autotracking.
CPs currently ignore autotracking, and have to depend on
argsmanuallyvia the
chainTagssystem. In the first attempt to support this we usedthe
UNKNOWN_PROPERTY_TAGAPI, but that actually doesn't work for thiscase because these aren't unknown properties, they do "exist" on the
proxy and should be enumerable.
Rather than adding another general API to the
tagForPropertyfunction,I opted to add some special logic to the
chainTagscrawler, similar towhat we do for
@each. When the chain is looking upargsas itssegment, it'll also check to see if the value is an args proxy, and if
so it'll use the (saved) CapturedNamedArgs directly. This check should
be low overhead since it's a string comparison, and only applies to CPs
and observers rather than all code (compared to adding it to
tagForProperty, which would affect new code as well).