Skip to content

Commit

Permalink
[BUGFIX] Do not eagerly consume modifier args during destruction
Browse files Browse the repository at this point in the history
Currently, in the 3.13 modifier capabilities version, we eagerly consume
the arguments that are passed to the modifier when it is destroying.
This can cause issues with backtracking rerender when the state isn't
actually used, and neither is the consumption. This change makes it so
that we don't eagerly consume these arguments during destruction in
general.
  • Loading branch information
Chris Garrett committed Feb 23, 2021
1 parent ddb199a commit 77a0d33
Show file tree
Hide file tree
Showing 4 changed files with 51 additions and 4 deletions.
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@
"typescript": "4.0.2"
},
"dependencies": {
"@handlebars/parser": "^2.0.0",
"@handlebars/parser": "~2.0.0",
"@simple-dom/document": "^1.4.0",
"@simple-dom/interface": "^1.4.0",
"@simple-dom/serializer": "^1.4.0",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,52 @@ abstract class ModifierManagerTest extends RenderTest {
/You attempted to update `foo` on `.*`, but it had already been used previously in the same computation/
);
}

@test
'does not eagerly access arguments during destruction'(assert: Assert) {
class Foo extends CustomModifier {}

let foo = this.defineModifier(Foo);

let Main = defineComponent(
{ foo },
'{{#if @state.show}}<h1 {{foo @state.bar [email protected]}}>hello world</h1>{{/if}}'
);

let barCount = 0;
let bazCount = 0;

class State {
@tracked show = true;

get bar() {
if (this.show === false) {
barCount++;
}

return;
}

get baz() {
if (this.show === false) {
bazCount++;
}

return;
}
}

let state = new State();

this.renderComponent(Main, { state });

state.show = false;

this.rerender();

assert.equal(barCount, 0, 'bar was not accessed during detruction');
assert.equal(bazCount, 0, 'baz was not accessed during detruction');
}
}

class ModifierManagerTest313 extends ModifierManagerTest {
Expand Down
5 changes: 3 additions & 2 deletions packages/@glimmer/manager/lib/public/modifier.ts
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,8 @@ export class CustomModifierManager<O extends Owner, ModifierInstance>

let { useArgsProxy, passFactoryToCreate } = delegate.capabilities;

let args = useArgsProxy ? argsProxyFor(capturedArgs, 'modifier') : reifyArgs(capturedArgs);
let argsProxy = argsProxyFor(capturedArgs, 'modifier');
let args = useArgsProxy ? argsProxy : reifyArgs(capturedArgs);

let instance: ModifierInstance;

Expand Down Expand Up @@ -168,7 +169,7 @@ export class CustomModifierManager<O extends Owner, ModifierInstance>
state.debugName = typeof definition === 'function' ? definition.name : definition.toString();
}

registerDestructor(state, () => delegate.destroyModifier(instance, state.args));
registerDestructor(state, () => delegate.destroyModifier(instance, argsProxy));

return state;
}
Expand Down
2 changes: 1 addition & 1 deletion yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -1529,7 +1529,7 @@
resolved "https://registry.yarnpkg.com/@glimmer/env/-/env-0.1.7.tgz#fd2d2b55a9029c6b37a6c935e8c8871ae70dfa07"
integrity sha1-/S0rVakCnGs3psk16MiHGucN+gc=

"@handlebars/parser@^2.0.0":
"@handlebars/parser@^2.0.0", "@handlebars/parser@~2.0.0":
version "2.0.0"
resolved "https://registry.yarnpkg.com/@handlebars/parser/-/parser-2.0.0.tgz#5e8b7298f31ff8f7b260e6b7363c7e9ceed7d9c5"
integrity sha512-EP9uEDZv/L5Qh9IWuMUGJRfwhXJ4h1dqKTT4/3+tY0eu7sPis7xh23j61SYUnNF4vqCQvvUXpDo9Bh/+q1zASA==
Expand Down

0 comments on commit 77a0d33

Please sign in to comment.