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 release] Ensure model can be observed by sync observers #18476

Merged
merged 1 commit into from
Oct 9, 2019

Conversation

pzuraq
Copy link
Contributor

@pzuraq pzuraq commented Oct 9, 2019

This is a compatibility change, otherwise sync observers
will not be able to watch the model property of controllers
at all. Also fixes the naming of the final flag to Observable#addObserver
and Observable#removeObserver

Fixes #18427

}

async '@test model can be observed with sync observers'(assert) {
let observerDidRun = false;
Copy link
Member

Choose a reason for hiding this comment

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

nit: may as well make this a counter and assert that it fired exactly once

model: EMBER_METAL_TRACKED_PROPERTIES ? tracked() : null,
model: computed({
get() {
return this[MODEL];
Copy link
Member

Choose a reason for hiding this comment

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

We may want to make this non-enumerable if there is an easy way to do it, but if it's too annoying it's not a big deal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only way to do it currently is something I think we'd rather deprecate sooner rather than later (it requires some not-so-nice prototype chain manipulation), so I'd rather not for the moment

Copy link
Member

@chancancode chancancode left a comment

Choose a reason for hiding this comment

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

Seems good to me

@pzuraq pzuraq force-pushed the bugfix/ensure-model-is-observable-sync branch from 93cf8c7 to 17b3e9a Compare October 9, 2019 17:16
This is a compatibility change, otherwise sync observers
will not be able to watch the model property of controllers
at all. Also fixes the naming of the final flag to Observable#addObserver
and Observable#removeObserver
@pzuraq pzuraq force-pushed the bugfix/ensure-model-is-observable-sync branch from 17b3e9a to 1d565fc Compare October 9, 2019 17:35
@pzuraq pzuraq merged commit fc9afa5 into master Oct 9, 2019
@pzuraq pzuraq deleted the bugfix/ensure-model-is-observable-sync branch October 9, 2019 18:41
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.13] Regression when observing model changes in controller
2 participants