Skip to content

Commit

Permalink
[BUGFIX] Fix reactivity of dynamic attributes
Browse files Browse the repository at this point in the history
The updates to use autotracking through the VM resulted in a bug in
dynamic attributes. Previously, the updating opcode for dynamic
attributes would run its update function based on tags/autotracking -
whenever any tag updated (e.g. a tracked value was changed) the
attribute would call its `update` function. The `update` function would
then check the new value against the current value of the attribute, and
update if they were different.

The bug was we introduced a value comparison in the updating opcode
itself, based on the last value of the property to set the attribute to.
This meant that if the property was updated to the same value twice in
a row, it would not call `update`, even if the attribute's value had
changed (e.g. because the user typed something).
  • Loading branch information
Chris Garrett committed Feb 12, 2021
1 parent ebd95ed commit 988a21e
Show file tree
Hide file tree
Showing 3 changed files with 58 additions and 15 deletions.
37 changes: 36 additions & 1 deletion packages/@glimmer/integration-tests/test/attributes-test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { normalizeProperty } from '@glimmer/runtime';
import { assertElement, hasAttribute, jitSuite, RenderTest, test } from '..';
import { assertElement, hasAttribute, jitSuite, RenderTest, test, tracked } from '..';
import { Namespace, SimpleElement } from '@simple-dom/interface';
import { castToBrowser, expect } from '@glimmer/util';

export class AttributesTests extends RenderTest {
static suiteName = 'Attributes';
Expand Down Expand Up @@ -239,6 +240,40 @@ export class AttributesTests extends RenderTest {
this.assertStableNodes();
}

@test
'handles successive updates to the same value'() {
class Model {
@tracked value = '';
}

let model = new Model();

this.render('<input value={{this.model.value}} />', { model });
this.assert.equal(this.readDOMAttr('value'), '');
this.assertStableRerender();

let inputElement = castToBrowser(
expect(this.element.firstChild, 'expected input to exist'),
'input'
);

inputElement.value = 'bar';
this.assert.equal(this.readDOMAttr('value'), 'bar');

model.value = 'foo';
this.rerender();
this.assert.equal(this.readDOMAttr('value'), 'foo');
this.assertStableNodes();

inputElement.value = 'bar';
this.assert.equal(this.readDOMAttr('value'), 'bar');

model.value = 'foo';
this.rerender();
this.assert.equal(this.readDOMAttr('value'), 'foo');
this.assertStableNodes();
}

@test
'input[checked] prop updates when set to undefined'() {
this.registerHelper('if', (params) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -544,7 +544,7 @@ function setDeferredAttr(
.elements()
.setDynamicAttribute(name, valueForRef(value), trusting, namespace);
if (!isConstRef(value)) {
vm.updateWith(new UpdateDynamicAttributeOpcode(value, attribute));
vm.updateWith(new UpdateDynamicAttributeOpcode(value, attribute, vm.env));
}
}
}
Expand Down
34 changes: 21 additions & 13 deletions packages/@glimmer/runtime/lib/compiled/opcodes/dom.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { Reference, valueForRef, isConstRef } from '@glimmer/reference';
import { Reference, valueForRef, isConstRef, createComputeRef } from '@glimmer/reference';
import { Revision, Tag, valueForTag, validateTag, consumeTag } from '@glimmer/validator';
import {
check,
Expand All @@ -17,6 +17,7 @@ import {
Owner,
CurriedType,
ModifierDefinitionState,
Environment,
} from '@glimmer/interfaces';
import { $t0 } from '@glimmer/vm';
import { APPEND_OPCODES, UpdatingOpcode } from '../../opcodes';
Expand Down Expand Up @@ -237,27 +238,34 @@ APPEND_OPCODES.add(Op.DynamicAttr, (vm, { op1: _name, op2: _trusting, op3: _name
let attribute = vm.elements().setDynamicAttribute(name, value, trusting, namespace);

if (!isConstRef(reference)) {
vm.updateWith(new UpdateDynamicAttributeOpcode(reference, attribute));
vm.updateWith(new UpdateDynamicAttributeOpcode(reference, attribute, vm.env));
}
});

export class UpdateDynamicAttributeOpcode extends UpdatingOpcode {
public type = 'patch-element';

public lastValue: unknown;
private updateRef: Reference;

constructor(private reference: Reference<unknown>, private attribute: DynamicAttribute) {
constructor(reference: Reference<unknown>, attribute: DynamicAttribute, env: Environment) {
super();
this.lastValue = valueForRef(reference);
}

evaluate(vm: UpdatingVM) {
let { attribute, reference, lastValue } = this;
let currentValue = valueForRef(reference);
let initialized = false;

if (currentValue !== lastValue) {
attribute.update(currentValue, vm.env);
this.lastValue = currentValue;
}
this.updateRef = createComputeRef(() => {
let value = valueForRef(reference);

if (initialized === true) {
attribute.update(value, env);
} else {
initialized = true;
}
});

valueForRef(this.updateRef);
}

evaluate() {
valueForRef(this.updateRef);
}
}

0 comments on commit 988a21e

Please sign in to comment.