Args not updating bug #67
Conversation
pittst3r
force-pushed
the
args-bug
branch
4 times, most recently
from
e2302d9
to
1450944
Compare
| @@ -31,7 +31,7 @@ | |||
| "@glimmer/util": "^0.26.2" | |||
| }, | |||
| "devDependencies": { | |||
| "@glimmer/application": "^0.5.0", | |||
| "@glimmer/application": "^0.7.2", | |||
There was a problem hiding this comment.
Is this upgrade necessary to fix the issue or can it be moved to a separate PR?
There was a problem hiding this comment.
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.
| app.scheduleRerender(); | ||
| }); | ||
|
|
||
| assert.equal(app.rootElement.textContent.trim(), 'Tom Dale is dope'); |
There was a problem hiding this comment.
A+++++ tests, would test again
test/args-test.ts
Outdated
| @@ -76,6 +77,106 @@ test('Args smoke test', (assert) => { | |||
| parent.firstName = "Thomas"; | |||
| }); | |||
|
|
|||
| test('Rendering args derivatives', (assert) => { | |||
There was a problem hiding this comment.
This is a nitpick but I would prefer test names that are more descriptive, so they are more accessible to future contributors. For example, perhaps instead of "Rendering args derivatives", we could say something like "Tracked properties that depend on args re-render correctly". The terse names make it difficult to understand at a glance the difference between the two tests.
| 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. |
There was a problem hiding this comment.
Just to verify, has the Glimmer VM bug that necessitated this been fixed? (Seems like yes.)
|
@tomdale Updated per review 😃 |
|
LGTM! 👍 Thank you for the hard work! |
Fixes #66.
The component manager is supposed to update
this.argson a component when they update. This was not happening at the appropriate time, causing computed properties dependent uponthis.argsto not update in time to reflect in the layout.Args were being updated in the
didUpdatecomponent manager hook but needed to be updated in theupdatehook. Updating the args indidUpdateis too late in the rendering process (didUpdateindicates that the component has updated already). Theupdatehook is for updating the component ahead of updating the layout so the layout knows what to render.Also upgrades the @glimmer/application devDependency and removes an unnecessary guard.