Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUGFIX beta] Fix issues with revalidation during teardown. #14110

Merged
merged 2 commits into from
Aug 23, 2016

Conversation

rwjblue
Copy link
Member

@rwjblue rwjblue commented Aug 22, 2016

When the toplevelView (aka ownerView) is being destroyed (during test teardown generally) an error was ocurring under certain "special" circumstances. Specifically, if removal of a sibling node triggered a revalidation of one still attached.

Consider the following example:

{{#x-select as |select|}}
 {{#select.option value="1"}}{/select.option}}
 {{#select.option value="2"}}{/select.option}}
{{/x-select}}

The components in question are using the registration pattern so that the children notify/register with the parent upon didInsertElement (and willDestroyElement).

When a new option is added or removed from the parent, it updates its value property which is bound to each options selected attribute (to toggle the selection state in the DOM).

The specific mechanism that causes the DOM to get updated when the various computed properties change is that a Stream object calls ownerNode.emberView.scheduleRevalidate. This tells the rest of the system to start walking the tree to flush out any updates to the DOM.

When a view is being removed, we set the emberView property of its renderNode to null before traversing the subtree of children. This means, that as the children are removed (and therefore cause the value of the parent to change) the selected attribute binding attempts to call ownerNode.emberView.scheduleRevalidation().

Unfortunately, when we are actually tearing down the ownerNode.emberView itself that invocation results in an error ( cannot call scheduleRevalidation on null).


This change adds a simple guard to avoid calling scheduleRevalidation when the ownerNode.emberView is being torn down.

Fixes #13846.
Fixes #13968.

@rwjblue
Copy link
Member Author

rwjblue commented Aug 22, 2016

I have uploaded the assets to make it a bit easier to test this in an ember-cli app. All you should need to do is change your bower.json from:

    "ember": "~2.8.0-beta.1",

To:

    "ember": "rwjblue/ember#pr-14110",

@rwjblue
Copy link
Member Author

rwjblue commented Aug 22, 2016

Since @krisselden worked on #13775, I'd love his review here...

@cibernox
Copy link
Contributor

I confirm that this branch fixes the test suite for Ember Power Select: https://travis-ci.org/cibernox/ember-power-select/jobs/154144524#L332

@rwjblue rwjblue force-pushed the fix-toplevel-tree-destruction branch from 2a64c3f to 3dd760c Compare August 22, 2016 13:52
When the toplevelView (aka `ownerView`) is being destroyed (during test
teardown generally) an error was ocurring under certain "special"
circumstances. Specifically, if removal of a sibling node triggered a
revalidation of one still attached.

Consider the following example:

```hbs
{{#x-select as |select|}}
  {{#select.option value="1"}}{/select.option}}
  {{#select.option value="2"}}{/select.option}}
{{/x-select}}
```

The components in question are using the registration pattern so that
the children notify/register with the parent upon `didInsertElement`
(and `willDestroyElement`).

When a new option is added or removed from the parent, it updates its
`value` property which is bound to each options `selected`
attribute (to toggle the selection state in the DOM).

The specific mechanism that causes the DOM to get updated when the
various computed properties change is that a Stream object calls
`ownerNode.emberView.scheduleRevalidate`.  This tells the rest of the
system to start walking the tree to flush out any updates to the DOM.

When a view is being removed, we set the `emberView`
property of its `renderNode` to `null` before traversing the subtree of
children. This means, that as the children are removed (and therefore
cause the `value` of the parent to change) the `selected` attribute
binding attempts to call `ownerNode.emberView.scheduleRevalidation()`.

Unfortunately, when we are actually tearing down the
`ownerNode.emberView` itself that invocation results in an error (
cannot call `scheduleRevalidation` on `null`).

---

This change adds a simple guard to avoid calling `scheduleRevalidation`
when the `ownerNode.emberView` is being torn down.
@backspace
Copy link

Thanks so much for this fix!

@cibernox
Copy link
Contributor

@rwjblue Could you port this commit to the beta branch so ember-try pulls the fix and addons got pretty green travis badges again 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants