Skip to content
This repository has been archived by the owner on Mar 5, 2018. It is now read-only.

Args not updating bug #67

Merged
merged 1 commit into from
Jul 11, 2017
Merged

Args not updating bug #67

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
"@glimmer/util": "^0.26.2"
},
"devDependencies": {
"@glimmer/application": "^0.5.0",
"@glimmer/application": "^0.7.2",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this upgrade necessary to fix the issue or can it be moved to a separate PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is necessary for these tests to pass. I could probably do this upgrade in a separate PR still, but it would need to be merged first.

"@glimmer/application-test-helpers": "^0.4.1",
"@glimmer/build": "^0.6.1",
"@glimmer/compiler": "^0.26.2",
Expand Down
10 changes: 2 additions & 8 deletions src/component-manager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,18 +133,12 @@ export default class ComponentManager implements GlimmerComponentManager<Compone
}

update(bucket: ComponentStateBucket, scope: DynamicScope) {
bucket.component.args = bucket.namedArgsSnapshot();
}

didUpdateLayout() {}

didUpdate(bucket: ComponentStateBucket) {
if (!bucket) { return; }

// TODO: This should be moved to `didUpdate`, but there's currently a
// Glimmer bug that causes it not to be called if the layout doesn't update.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to verify, has the Glimmer VM bug that necessitated this been fixed? (Seems like yes.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

let { component } = bucket;

component.args = bucket.namedArgsSnapshot();
didUpdate({ component }: ComponentStateBucket) {
component.didUpdate();
}

Expand Down
100 changes: 100 additions & 0 deletions test/args-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import Component from '../src/component';
import { tracked } from '../src/tracked';
import buildApp, { TestApplication } from './test-helpers/test-app';
import { setPropertyDidChange } from '../src/tracked';
import { didRender } from '@glimmer/application-test-helpers';

const { module, test } = QUnit;

Expand Down Expand Up @@ -76,6 +77,105 @@ test('Args smoke test', (assert) => {
parent.firstName = "Thomas";
});

test('Tracked properties that depend on `args` re-render correctly', (assert) => {
assert.expect(2);

let parent: ParentComponent;
let app: TestApplication;

class ParentComponent extends Component {
@tracked firstName = 'Tom';
@tracked status = 'is dope';

didInsertElement() {
parent = this;
}
}

class ChildComponent extends Component {
@tracked('args') get name() {
return `${this.args.firstName} Dale`;
}
}

app = buildApp()
.component('parent-component', ParentComponent)
.component('child-component', ChildComponent)
.template('main', '<div><parent-component /></div>')
.template('parent-component', `
<div>
<child-component @firstName={{firstName}} @status={{status}} />
</div>`)
.template('child-component', '<div>{{name}} {{@status}}</div>')
.boot();

setPropertyDidChange(function() {
app.scheduleRerender();
});

assert.equal(app.rootElement.textContent.trim(), 'Tom Dale is dope');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A+++++ tests, would test again

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, Thom.


parent.firstName = 'Thom';
parent.status = 'is dank';

return didRender(app).then(() => {
assert.equal(app.rootElement.textContent.trim(), 'Thom Dale is dank');
});
});

test('Properties that depend on `args` are properly updated before the `didUpdate` hook', (assert) => {
assert.expect(4);

let parent: ParentComponent;
let app: TestApplication;

class ParentComponent extends Component {
@tracked firstName = 'Tom';
@tracked status = 'is dope';

didInsertElement() {
parent = this;
}
}

class ChildComponent extends Component {
get name() {
return `${this.args.firstName} Dale`;
}

constructor(injections: object) {
super(injections);
assert.equal(this.name, 'Tom Dale');
assert.equal(this.args.status, 'is dope');
}

didUpdate() {
assert.equal(this.name, 'Thom Dale');
assert.equal(this.args.status, 'is dank');
}
}

app = buildApp()
.component('parent-component', ParentComponent)
.component('child-component', ChildComponent)
.template('main', '<div><parent-component /></div>')
.template('parent-component', `
<div>
<child-component @firstName={{firstName}} @status={{status}} />
</div>`)
.template('child-component', '<div></div>')
.boot();

setPropertyDidChange(function() {
app.scheduleRerender();
});

parent.firstName = 'Thom';
parent.status = 'is dank';

return didRender(app);
});

test("Setting args should not schedule a rerender", function(assert) {
let done = assert.async();
let app: TestApplication;
Expand Down
76 changes: 15 additions & 61 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,18 +18,19 @@
"@glimmer/wire-format" "^0.26.2"
simple-dom "^0.3.2"

"@glimmer/application@^0.5.0":
version "0.5.0"
resolved "https://registry.yarnpkg.com/@glimmer/application/-/application-0.5.0.tgz#4f5b39de2bd099d1c1f07b237515a38698c622ec"
"@glimmer/application@^0.7.2":
version "0.7.2"
resolved "https://registry.npmjs.org/@glimmer/application/-/application-0.7.2.tgz#82cc78367f54c11c953c8624cba92425473e1d88"
dependencies:
"@glimmer/component" "^0.4.0"
"@glimmer/compiler" "^0.26.2"
"@glimmer/component" "^0.6.0"
"@glimmer/di" "^0.1.9"
"@glimmer/env" "^0.1.7"
"@glimmer/object-reference" "^0.24.0-beta.4"
"@glimmer/reference" "^0.24.0-beta.4"
"@glimmer/object-reference" "^0.26.2"
"@glimmer/reference" "^0.26.2"
"@glimmer/resolver" "^0.3.0"
"@glimmer/runtime" "^0.24.0-beta.4"
"@glimmer/util" "^0.24.0-beta.4"
"@glimmer/runtime" "^0.26.2"
"@glimmer/util" "^0.26.2"

"@glimmer/build@^0.6.1":
version "0.6.1"
Expand Down Expand Up @@ -83,15 +84,15 @@
"@glimmer/wire-format" "^0.26.2"
simple-html-tokenizer "^0.3.0"

"@glimmer/component@^0.4.0":
version "0.4.0"
resolved "https://registry.yarnpkg.com/@glimmer/component/-/component-0.4.0.tgz#2cd491045f4d86502cf4f3249f8f6c51de046b0c"
"@glimmer/component@^0.6.0":
version "0.6.0"
resolved "https://registry.npmjs.org/@glimmer/component/-/component-0.6.0.tgz#60b88afb7ba87a9d30d44fe2dd167609be1e19cc"
dependencies:
"@glimmer/di" "^0.1.9"
"@glimmer/env" "^0.1.7"
"@glimmer/reference" "^0.24.0-beta.4"
"@glimmer/runtime" "^0.24.0-beta.4"
"@glimmer/util" "^0.24.0-beta.4"
"@glimmer/reference" "^0.26.2"
"@glimmer/runtime" "^0.26.2"
"@glimmer/util" "^0.26.2"

"@glimmer/di@^0.1.9":
version "0.1.11"
Expand All @@ -105,52 +106,26 @@
version "0.1.7"
resolved "https://registry.yarnpkg.com/@glimmer/env/-/env-0.1.7.tgz#fd2d2b55a9029c6b37a6c935e8c8871ae70dfa07"

"@glimmer/interfaces@^0.24.0-beta.4":
version "0.24.0-beta.4"
resolved "https://registry.yarnpkg.com/@glimmer/interfaces/-/interfaces-0.24.0-beta.4.tgz#c4ee238d22f9d6805328e986326fcaa042843878"
dependencies:
"@glimmer/wire-format" "^0.24.0-beta.4"

"@glimmer/interfaces@^0.26.2":
version "0.26.2"
resolved "https://registry.npmjs.org/@glimmer/interfaces/-/interfaces-0.26.2.tgz#f03e7309584006de164b82ff924692b210888e19"
dependencies:
"@glimmer/wire-format" "^0.26.2"

"@glimmer/object-reference@^0.24.0-beta.4":
version "0.24.0-beta.4"
resolved "https://registry.yarnpkg.com/@glimmer/object-reference/-/object-reference-0.24.0-beta.4.tgz#0f10ef9f9540f782a9b3d6014b1da4d159557014"
dependencies:
"@glimmer/reference" "^0.24.0-beta.4"
"@glimmer/util" "^0.24.0-beta.4"

"@glimmer/object-reference@^0.26.2":
version "0.26.2"
resolved "https://registry.npmjs.org/@glimmer/object-reference/-/object-reference-0.26.2.tgz#df3688d3a0003775042e66c06669b5b183503b36"
dependencies:
"@glimmer/reference" "^0.26.2"
"@glimmer/util" "^0.26.2"

"@glimmer/object@^0.24.0-beta.4":
version "0.24.0-beta.4"
resolved "https://registry.yarnpkg.com/@glimmer/object/-/object-0.24.0-beta.4.tgz#df272461fcd2abd1736b8eae9e2f10a918d9e4b9"
dependencies:
"@glimmer/object-reference" "^0.24.0-beta.4"
"@glimmer/util" "^0.24.0-beta.4"

"@glimmer/object@^0.26.2":
version "0.26.2"
resolved "https://registry.npmjs.org/@glimmer/object/-/object-0.26.2.tgz#a6a99d88bc4acd5ce9f2e46a62b618b15e7d5bfa"
dependencies:
"@glimmer/object-reference" "^0.26.2"
"@glimmer/util" "^0.26.2"

"@glimmer/reference@^0.24.0-beta.4":
version "0.24.0-beta.4"
resolved "https://registry.yarnpkg.com/@glimmer/reference/-/reference-0.24.0-beta.4.tgz#17f32cf1a4df3ebf57ca62647fc578b608b9f62d"
dependencies:
"@glimmer/util" "^0.24.0-beta.4"

"@glimmer/reference@^0.26.2":
version "0.26.2"
resolved "https://registry.npmjs.org/@glimmer/reference/-/reference-0.26.2.tgz#b14ad1180559e51353519ee62f21aabad737b0cc"
Expand All @@ -163,17 +138,6 @@
dependencies:
"@glimmer/di" "^0.2.0"

"@glimmer/runtime@^0.24.0-beta.4":
version "0.24.0-beta.4"
resolved "https://registry.yarnpkg.com/@glimmer/runtime/-/runtime-0.24.0-beta.4.tgz#dd1f525b7545f4ed9ef5d9a2bbe586e3db1d7a55"
dependencies:
"@glimmer/interfaces" "^0.24.0-beta.4"
"@glimmer/object" "^0.24.0-beta.4"
"@glimmer/object-reference" "^0.24.0-beta.4"
"@glimmer/reference" "^0.24.0-beta.4"
"@glimmer/util" "^0.24.0-beta.4"
"@glimmer/wire-format" "^0.24.0-beta.4"

"@glimmer/runtime@^0.26.2":
version "0.26.2"
resolved "https://registry.npmjs.org/@glimmer/runtime/-/runtime-0.26.2.tgz#2629259b619828bb058b8d9bc05a7c745ba53b05"
Expand All @@ -194,20 +158,10 @@
handlebars "^4.0.6"
simple-html-tokenizer "^0.3.0"

"@glimmer/util@^0.24.0-beta.4":
version "0.24.0-beta.4"
resolved "https://registry.yarnpkg.com/@glimmer/util/-/util-0.24.0-beta.4.tgz#d49063e432e2fd2af7bdc1422d3b0d849e6ce328"

"@glimmer/util@^0.26.2":
version "0.26.2"
resolved "https://registry.npmjs.org/@glimmer/util/-/util-0.26.2.tgz#829e0cff7cee767689267f37533ea789da0bcc36"

"@glimmer/wire-format@^0.24.0-beta.4":
version "0.24.0-beta.4"
resolved "https://registry.yarnpkg.com/@glimmer/wire-format/-/wire-format-0.24.0-beta.4.tgz#793acc653e2b718b5052f59943604c0317995b75"
dependencies:
"@glimmer/util" "^0.24.0-beta.4"

"@glimmer/wire-format@^0.26.2":
version "0.26.2"
resolved "https://registry.npmjs.org/@glimmer/wire-format/-/wire-format-0.26.2.tgz#5ff95a61c32c18bc65bd5b86af74bcbdd38e62d9"
Expand Down