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 lts] Ensure hash objects correctly entangle as dependencies #19583

Closed
wants to merge 1 commit into from

Conversation

pzuraq
Copy link
Contributor

@pzuraq pzuraq commented Jun 1, 2021

The previous bugfixes to {{hash}} caused a change to the semantics of
computed properties that depend on a hash. Specifically, because
{{hash}} objects are now proxies, they are constant, never updating
again after they are initially created. This is fine if you depend on
an individual key in a hash, but breaks if you depend directly on the
hash itself:

computed('hash.foo', function() {}) // this works

computed('hash', function() {}) // this will no longer rerun

This is used occasionally when you wish to depend on the dynamic keys
of a dictionary, like so:

computed('hash', function() {
  let values = [];

  for (let key in this.hash) {
    values.push(hash[key]);
  }

  return values;
})

Notably, this is not a problem with autotracking, because autotracking
will entangle the usage of these keys dynamically. So this is only a
problem with legacy systems such as computed and observer which
cannot dynamically add dependencies based on the function's runtime.

To fix this, we need to determine if a dependency is a hash when a
computed or an observer depends upon it, and then entangle all of its
keys if it is. Unfortunately, we cannot do this by just checking what
the value is, because we do not load the value of leaf dependencies
(the last segment of a dependency) as a perf optimization.

This tries to instead create a new type of "ComputedProperty" that gets
installed whenever a value is set to be a HashProxy. This gives us a
way to load the value of a property only when it is a hash, or when it
was set to a hash at least once.

The previous bugfixes to `{{hash}}` caused a change to the semantics of
computed properties that depend on a hash. Specifically, because
`{{hash}}` objects are now proxies, they are _constant_, never updating
again after they are initially created. This is fine if you depend on
an individual key in a hash, but breaks if you depend directly on the
hash itself:

```js
computed('hash.foo', function() {}) // this works

computed('hash', function() {}) // this will no longer rerun
```

This is used occasionally when you wish to depend on the dynamic keys
of a dictionary, like so:

```js
computed('hash', function() {
  let values = [];

  for (let key in this.hash) {
    values.push(hash[key]);
  }

  return values;
})
```

Notably, this is not a problem with autotracking, because autotracking
will entangle the usage of these keys dynamically. So this is only a
problem with legacy systems such as `computed` and `observer` which
cannot dynamically add dependencies based on the function's runtime.

To fix this, we need to determine if a dependency is a hash when a
computed or an observer depends upon it, and then entangle all of its
keys if it is. Unfortunately, we cannot do this by just checking what
the value is, because we do not load the value of leaf dependencies
(the last segment of a dependency) as a perf optimization.

This tries to instead create a new type of "ComputedProperty" that gets
installed whenever a value is set to be a HashProxy. This gives us a
way to load the value of a property _only_ when it is a hash, or when it
was set to a hash at least once.
@pzuraq
Copy link
Contributor Author

pzuraq commented Jun 1, 2021

Depends on glimmerjs/glimmer-vm#1319

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.

None yet

1 participant