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.

Sign up for GitHub

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

Jump to bottom

[BUGFIX lts] Ensure setter CP's with dependent keys on curly components can be two way bound #19028

Merged
merged 7 commits into from Aug 5, 2020
Merged

[BUGFIX lts] Ensure setter CP's with dependent keys on curly components can be two way bound #19028

merged 7 commits into from Aug 5, 2020

Conversation

richgt
Copy link
Contributor

@richgt richgt commented Jun 25, 2020

No description provided.

@richgt richgt mentioned this pull request Jun 25, 2020
Closed
@richgt @rwjblue
Add test case for two way binding through a setter CP.
acc8100 
Co-authored-by: Robert Jackson <me@rwjblue.com>
@rwjblue rwjblue force-pushed the richgt/18147-computed-property-issue branch from 6f70130 to acc8100 Compare June 25, 2020 19:32
@richgt richgt force-pushed the richgt/18147-computed-property-issue branch from 953406e to 7e6a91b Compare June 25, 2020 20:40
@richgt richgt force-pushed the richgt/18147-computed-property-issue branch from 7e6a91b to ae0c5d8 Compare June 25, 2020 20:59
@richgt richgt changed the title GH#18147 - add test to show Two Way Bound computed property issue GH#18417 - add test to show Two Way Bound computed property issue Jun 25, 2020
@richgt richgt force-pushed the richgt/18147-computed-property-issue branch from 472e373 to 5a7e166 Compare June 25, 2020 22:05
@rwjblue rwjblue changed the title GH#18417 - add test to show Two Way Bound computed property issue [BUGFIX lts] Ensure setter CP's with dependent keys on curly components can be two way bound Jun 29, 2020
@rwjblue
Copy link
Member

rwjblue commented Jun 29, 2020

Added a few reviewers...

@richgt
Copy link
Contributor Author

richgt commented Jul 6, 2020

@rwjblue - we have some scenarios where this appears to not completely fix the issue. We're working on further small-scale reproduction, as we can't quite extract it from our application code, but we do have several other instances of computed properties still not successfully updating up to their two-way bound parents. I'll let you know if we're able to successfully provide smaller-scale reproductions.

@richgt
Copy link
Contributor Author

richgt commented Jul 6, 2020

@rwjblue - we have some scenarios where this appears to not completely fix the issue. We're working on further small-scale reproduction, as we can't quite extract it from our application code, but we do have several other instances of computed properties still not successfully updating up to their two-way bound parents. I'll let you know if we're able to successfully provide smaller-scale reproductions.

Gut instinct right now tells me it has something to do with two-way binding to properties inside an object (twb-value=object.value). Again, still haven't come away with a better reproduction, but we're continuing to work on it. It's pretty consistent across our codebase. Will update when I have more.

@pzuraq
Copy link
Contributor

pzuraq commented Jul 13, 2020

does the observer have to be a sync observer? Can it be async?

@rwjblue
Copy link
Member

rwjblue commented Jul 13, 2020

does the observer have to be a sync observer? Can it be async?

Ya probably, I'll test.

@rwjblue
Copy link
Member

rwjblue commented Jul 13, 2020

After discussing with @pzuraq (offline) I think I'm also going to see if it is possible to do from the CP side (instead of the curly component manager side). Specifically, the thought process is that we instantiate many more @ember/component instances than (we expect) folks use setter CP's with dependent keys. The current implementation introduces a cost for every component instantiation (we have to loop over all the named arguments, check if that ran into a setter CP that used dep keys, and then add the observer).

@richgt richgt force-pushed the richgt/18147-computed-property-issue branch 3 times, most recently from 01f7224 to 1f35753 Compare August 3, 2020 01:23
@richgt richgt force-pushed the richgt/18147-computed-property-issue branch from 1f35753 to f2e8c2d Compare August 3, 2020 01:32
rwjblue
rwjblue requested changes Aug 3, 2020
View reviewed changes
packages/@ember/-internals/metal/lib/computed.ts Outdated Show resolved Hide resolved
packages/@ember/-internals/metal/lib/computed.ts Outdated Show resolved Hide resolved
packages/@ember/-internals/metal/lib/computed.ts Outdated Show resolved Hide resolved
packages/@ember/-internals/metal/lib/computed.ts Outdated Show resolved Hide resolved
richgt
richgt commented Aug 3, 2020
View reviewed changes
Copy link
Contributor Author

@richgt richgt left a comment

Choose a reason for hiding this comment

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

LGTM

@rwjblue
Refactor computed solution
dbebd35 
* Change guard for setting observer up to use `this` instead of
  `descriptorForProperty`
* Check for `obj[PROPERTY_DID_CHANGE]` and `obj.isComponent` instead of
  `instanceof Ember.Component`
* Move observer setup outside of `try`/`finally`
* Use arrow function for observer (instead of `Function.prototype.bind`)
@rwjblue rwjblue force-pushed the richgt/18147-computed-property-issue branch from d6f61f5 to dbebd35 Compare August 3, 2020 15:14
@rwjblue
Copy link
Member

rwjblue commented Aug 3, 2020

Chatted a bit with @krisselden about the updated fix here, we need to do a (hopefully) quick performance test to confirm this does not introduce a regression for existing apps. I believe that @pzuraq mentioned that he would run the comparison.

@pzuraq
Copy link
Contributor

pzuraq commented Aug 5, 2020

Benchmarked, and this doesn't appear to have a significant impact. We're good to merge! Thanks a ton @richgt!

artifact-49.pdf

@pzuraq pzuraq merged commit 3ef8495 into emberjs:master Aug 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@boris-petrov boris-petrov boris-petrov left review comments

@rwjblue rwjblue rwjblue approved these changes

@pzuraq pzuraq pzuraq approved these changes

@chancancode chancancode Awaiting requested review from chancancode

@krisselden krisselden Awaiting requested review from krisselden

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@richgt @rwjblue @pzuraq @boris-petrov