Skip to content

Commit

Permalink
[BUGFIX lts] Ensure hash objects correctly entangle as dependencies v2
Browse files Browse the repository at this point in the history
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. We do this whenever the value is the last value in the
chain.
  • Loading branch information
Chris Garrett committed Jun 21, 2021
1 parent b4ace38 commit 03b20d8
Show file tree
Hide file tree
Showing 4 changed files with 174 additions and 155 deletions.
26 changes: 13 additions & 13 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@
"@babel/plugin-transform-block-scoping": "^7.8.3",
"@babel/plugin-transform-object-assign": "^7.8.3",
"@ember/edition-utils": "^1.2.0",
"@glimmer/vm-babel-plugins": "0.79.3",
"@glimmer/vm-babel-plugins": "0.79.4",
"babel-plugin-debug-macros": "^0.3.3",
"babel-plugin-filter-imports": "^4.0.0",
"broccoli-concat": "^4.2.4",
Expand All @@ -75,19 +75,19 @@
},
"devDependencies": {
"@babel/preset-env": "^7.9.5",
"@glimmer/compiler": "0.79.3",
"@glimmer/destroyable": "0.79.3",
"@glimmer/compiler": "0.79.4",
"@glimmer/destroyable": "0.79.4",
"@glimmer/env": "^0.1.7",
"@glimmer/global-context": "0.79.3",
"@glimmer/interfaces": "0.79.3",
"@glimmer/manager": "0.79.3",
"@glimmer/node": "0.79.3",
"@glimmer/opcode-compiler": "0.79.3",
"@glimmer/owner": "0.79.3",
"@glimmer/program": "0.79.3",
"@glimmer/reference": "0.79.3",
"@glimmer/runtime": "0.79.3",
"@glimmer/validator": "0.79.3",
"@glimmer/global-context": "0.79.4",
"@glimmer/interfaces": "0.79.4",
"@glimmer/manager": "0.79.4",
"@glimmer/node": "0.79.4",
"@glimmer/opcode-compiler": "0.79.4",
"@glimmer/owner": "0.79.4",
"@glimmer/program": "0.79.4",
"@glimmer/reference": "0.79.4",
"@glimmer/runtime": "0.79.4",
"@glimmer/validator": "0.79.4",
"@simple-dom/document": "^1.4.0",
"@types/qunit": "^2.11.1",
"@types/rsvp": "^4.0.3",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -350,6 +350,30 @@ moduleFor(
assert.equal(theLink.attr('href'), '/?foo=ASL');
}

async ['@test supplied QP properties can be bound in legacy components'](assert) {
expectDeprecation(/Passing the `@tagName` argument to/);

this.addTemplate(
'index',
`
<LinkTo @tagName="a" id="the-link" @query={{hash foo=this.boundThing}}>
Index
</LinkTo>
`
);

await this.visit('/');

let indexController = this.getController('index');
let theLink = this.$('#the-link');

assert.equal(theLink.attr('href'), '/?foo=OMG');

runTask(() => indexController.set('boundThing', 'ASL'));

assert.equal(theLink.attr('href'), '/?foo=ASL');
}

async ['@test supplied QP properties can be bound (booleans)'](assert) {
this.addTemplate(
'index',
Expand Down
27 changes: 11 additions & 16 deletions packages/@ember/-internals/metal/lib/chain-tags.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { Meta, meta as metaFor, peekMeta } from '@ember/-internals/meta';
import { isObject } from '@ember/-internals/utils';
import { assert, deprecate } from '@ember/debug';
import { isHashProxy } from '@glimmer/runtime';
import { _WeakSet } from '@glimmer/util';
import {
combine,
Expand All @@ -16,7 +17,7 @@ import { tagForProperty } from './tags';

export const CHAIN_PASS_THROUGH = new _WeakSet();

export function finishLazyChains(meta: Meta, key: string, value: any) {
export function finishLazyChains(meta: Meta, key: string, value: unknown): void {
let lazyTags = meta.readableLazyChainsFor(key);

if (lazyTags === undefined) {
Expand Down Expand Up @@ -173,20 +174,6 @@ function getChainTags(

chainTags.push(propertyTag);

// If we're at the end of the path, processing the last segment, and it's
// not an alias, we should _not_ get the last value, since we already have
// its tag. There's no reason to access it and do more work.
if (segmentEnd === pathLength) {
// If the key was an alias, we should always get the next value in order to
// bootstrap the alias. This is because aliases, unlike other CPs, should
// always be in sync with the aliased value.
if (CHAIN_PASS_THROUGH.has(descriptor)) {
// tslint:disable-next-line: no-unused-expression
current[segment];
}
break;
}

if (descriptor === undefined) {
// If the descriptor is undefined, then its a normal property, so we should
// lookup the value to chain off of like normal.
Expand Down Expand Up @@ -223,8 +210,16 @@ function getChainTags(
}
}

if (!isObject(current)) {
if (segmentEnd === pathLength || !isObject(current)) {
// we've hit the end of the chain for now, break out

// If the last value is a HashProxy, then entangle all of its tags
if (isHashProxy(current)) {
for (let key in current) {
chainTags.push(tagForProperty(current, key));
}
}

break;
}

Expand Down
Loading

0 comments on commit 03b20d8

Please sign in to comment.