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] Fix issue #14672 #14681

Merged
merged 1 commit into from
Dec 6, 2016
Merged

[BUGFIX release] Fix issue #14672 #14681

merged 1 commit into from
Dec 6, 2016

Conversation

krisselden
Copy link
Contributor

PROPERTY_DID_CHANGE is the mechanism by which Ember's Glimmer refs are dirtied. Currently if something watches then unwatches an alias, the DK that is installed by the alias is removed.

@krisselden krisselden changed the title Failing test for #14672 [BUGFIX release] Fix issue #14672 Dec 6, 2016
if (cache[keyName] !== CONSUMED) {
cache[keyName] = CONSUMED;
addDependentKeys(this, obj, keyName, meta);
}
return get(obj, this.altKey);
Copy link
Member

Choose a reason for hiding this comment

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

the above behavior is slightly different then how regular CP work: https://github.com/emberjs/ember.js/blob/master/packages/ember-metal/lib/computed.js#L338-L349

  • we add the DK first, then invoke the get. Should we follow the pattern of regular CP, add the DK after the first get?
  • should get on alias be cached, until the DK changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll change the order to be consistent, though getters really shouldn't have side effects. As for caching, it is likely cached on the other side, I'm just trying to track unconsumed vs not.

Copy link
Member

Choose a reason for hiding this comment

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

though getters really shouldn't have side effects.

They shouldn't but they often do, especially in development mode where we toString objects, and users can provide there own toString

Not a big deal though, only doit if you are ok with it.

@stefanpenner
Copy link
Member

:shipit:

@krisselden
Copy link
Contributor Author

Trying to push an update to this but the push failed and but trying again says everything is up to date.

PROPERTY_DID_CHANGE is the mechanism by which
Ember's Glimmer refs are dirtied.  Currently if
something watches then unwatches an alias, the DK
that is installed by the alias is removed.
@krisselden krisselden merged commit ef5755c into master Dec 6, 2016
@homu homu mentioned this pull request Dec 6, 2016
@chadhietala chadhietala deleted the issue-14672 branch December 6, 2016 22:16
@chadhietala chadhietala restored the issue-14672 branch December 6, 2016 22:16
@stefanpenner stefanpenner deleted the issue-14672 branch December 7, 2016 00:54
@medokin
Copy link

medokin commented Dec 7, 2016

@krisselden Will that come into the next 2.10 patch?

@stefanpenner
Copy link
Member

@medokin yes

@chancancode
Copy link
Member

@medokin released in 2.10.1 😄

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.

4 participants