Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUGFIX release] Backport autotracking bugfixes #18721

Merged
merged 4 commits into from
Feb 3, 2020

Conversation

pzuraq
Copy link
Contributor

@pzuraq pzuraq commented Feb 1, 2020

No description provided.

Chris Garrett added 3 commits February 3, 2020 10:42
Previously, the arg proxy used a custom system for passing along its
`capturedArgs` directly to `getChainTags`, in order to interoperate with
computed properties. As it turns out, there are a number of other places
where the correct tag needs to be gotten and setup.

This PR replaces the `UNKNOWN_PROPERTY_TAG` system with a more general
`CUSTOM_TAG_FOR` system. In this system, if we detect that an object has
this method, we defer to it to get the tag, even if the property was
defined on the object. There are two users of this system:

- ProxyMixin
- The arg proxy

Given that it's private, and we have relatively few use cases, I believe
this is the cleanest solution at the moment. The alternative would be to
keep `UNKNOWN_PROPERTY_TAG` and also add `CUSTOM_TAG_FOR`, but that
seems unnecessary given low usage.
Currently, the Proxy mixin uses the private `UNKNOWN_PROPERTY_TAG` API
to provide the tags for the content property that the mixin is proxying
to. However, previously the chains system would do this _and_ still
entangle changes to the local property. This allowed users to manually
notify in subclasses of Proxy.

This PR adds the tag for the property back to the returned tag, so it
works the same as before. It also refactors `setupMandatorySetter`,
since we now have to do that work in two places (to avoid re-entry).
Currently, if `arrangedContent` is overridden in an ArrayProxy with a
computed property that depends on changes to another array/context,
those changes will not propagate correctly. This is because we never
link the tags of the ArrayProxy to the corresponding tags of the
`arrangedContent`, instead relying on array observers to propagate
changes. This works when the underlying array is being changed directly,
but _doesn't_ work if the array is being replaced entirely (e.g. the
computed property has invalidated and needs to recompute).

This PR ensures that ArrayProxy tags are setup correctly, so that if
`arrangedContent` ever changes, the proxy will also propagate those
changes. This will affect anything that depends on the ArrayProxy
directly, such as `{{#each}}` loops and other computed properties.

One side effect of this is that ArrayProxy's no longer need to manually
dirty themselves, and in fact attempting to do so can trigger the
backtracking rerender assertion (specifically when the proxy first
attempts to update/synchronize while rendering). Internally, a boolean
flag has been added to the array change methods to allow it to opt-out
of sending a notification.
@rwjblue rwjblue force-pushed the backport/autotracking-bugfixes branch from 7f9edd7 to 63f9f45 Compare February 3, 2020 15:45
@rwjblue rwjblue changed the title [BUGFIX release] Backport autotracking bugfixes Merge pull request #18721 from emberjs/backport/autotracking-bugfixes Feb 3, 2020
@rwjblue rwjblue merged commit 4f10138 into release Feb 3, 2020
@rwjblue rwjblue deleted the backport/autotracking-bugfixes branch February 3, 2020 17:35
@rwjblue rwjblue changed the title Merge pull request #18721 from emberjs/backport/autotracking-bugfixes [BUGFIX release] Backport autotracking bugfixes Feb 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants