[BUGFIX beta] allow watching of ES5+ Getter #12491

stefanpenner · 2015-10-16T22:22:22Z

Before this commit MandatorySetter enabled + current property being watched
resulted in get always fetching via meta.values. Although this typically works,
the default getter for a MandatorySetter already does this, so it is actually redundant.

Going further, when a user defines their own getter this code-path is incorrectly taken,
instead the original value (which originated from the getter) should be the consulted value.

This is blocking an ember-data fix, and likely causing grief for users who use ES5 getter/setters mixed in with our templates.

cc @krisselden

rwjblue · 2015-10-16T22:53:34Z

Looks great! Can you add a test to https://github.com/emberjs/ember.js/blob/master/packages/ember-metal/tests/accessors/mandatory_setters_test.js also?

I think this should likely be bug fix beta to allow for our normal beta testing cycle.

stefanpenner · 2015-10-16T22:58:05Z

I think this should likely be bug fix beta to allow for our normal beta testing cycle.

This is breaking things in dev mode, and doesn't do anything radical to solve the issues. That being said, it may not be a critical fix on-our end.

@bmac is it alright if ember-data doesn't get this solution till 2.2?

rwjblue · 2015-10-16T23:04:50Z

is breaking things in dev mode, and doesn't do anything radical to solve the issues. That being said, it may not be a critical fix on-our end.

Yup, I understand that the bug exists in previously released versions (not just beta). Unless things are fundamentally broken in the wild (which would likely result in a large uproar) we should err on the side of using the normal beta process. This change is deep in the internals and a mistake in this area has the ability to cause massive issues that we may not be able to predict (we have had a number of [BUGFIX release] changes come back to bite us in the backside). Thankfully, this is exactly why we have a beta cycle.

stefanpenner · 2015-10-16T23:06:59Z

@rwjblue yup just stating facts & checking with @bmac to see if this is critical or not to ed. If it is not, its good to go on beta.

stefanpenner · 2015-10-17T00:11:06Z

fix more stuff, our tests here aren't very good..

bmac · 2015-10-17T06:44:12Z

@stefanpenner this functionality is not critical for Ember Data. We shipped a work around for this issue in 2.1 so we can refactor the workaround away in a future release when Ember's support improves.

stefanpenner · 2015-10-18T15:02:17Z

@bmac I think this addresses everything I could think of, if you have time to confirm on your end that would be good.

––––

Turned out, as I worked on this I realized many other things were funky. I suspect this code can continue to be improved.

@rwjblue r?
@krisselden r?

stefanpenner · 2015-10-18T15:05:53Z

b66a13f removed keys_test and moved them, I added some to that file, i should move them aswell

mixonic · 2015-10-18T15:15:07Z

So many new tests 👏

stefanpenner · 2015-10-18T15:21:27Z

Test failures seem related to phantom, will investigate soon

rwjblue · 2015-10-18T16:09:52Z

packages/ember-metal/lib/properties.js

@@ -38,6 +41,16 @@ export function DEFAULT_GETTER_FUNCTION(name) {
  };
 }

+export function INHERITING_GETTER_FUNCTION(name) {
+  function IGETTER_FUNCTION() {
+    var proto = Object.getPrototypeOf(this);


It seems to me (just off the bat) that we need to do the same thing as was added in #12314 to lookup the getter through the entire prototype chain. As it exists here, I believe that a getter from a parent class will not be found.

Specifically, I think we should use lookupDescriptor, and add some tests around this behavior like these ones only for getter.

I don't think so, this exists to simulate the behavior if no descriptor existed at this level. It aims to re-target get one step up the prototype chain.

In-case i mis-understood, here are the relevant tests (which may explain better when this aims to do then I can), -> Related tests->: https://github.com/emberjs/ember.js/pull/12491/files#diff-25946da80206a01a2f680ad806bb1780R370

Let me know if im not crazy.

wait i believe i see it now.

adding another test

Added a test, will investigate another time (time for bed)

stefanpenner · 2015-10-24T22:14:12Z

After chatting with kris, he had a good idea, installing the mandatory setter for inherited properties in at the level defined (instead of at the leaf). Will explore this.

Before this commit MandatorySetter enabled + current property being watched resulted in `get` always fetching via meta.values. Although this typically works, the default getter for a MandatorySetter already does this, so it is actually redundant. Going further, when a user defines their own getter this code-path is incorrectly taken, instead the original value (which originated from the getter) should be the consulted value

[fixes 12480]

* no longer rely on meta to store the value until next set after uninstalling MandatorySetter * no longer incorrectly re-set on set to enumerable (enumerability should remain constant) * test both enumerable / non-enumerable variants

stefanpenner · 2016-01-11T00:27:59Z

@rwjblue @krisselden r?

stefanpenner · 2016-01-11T00:29:21Z

I don't believe the IE9 failure is critical, but I will investigate it once I have IE9 handy. It appears to just be a IE9ism, and very specific tests one my part. I just need to go with IE9 and see how it handles this edge case and update the tests accordingly.

stefanpenner · 2016-01-11T18:42:09Z

IE9 issue is fixed in my VM, lets see what sauce thinks.

stefanpenner · 2016-01-11T19:49:08Z

stefanpenner · 2016-01-20T00:09:23Z

It is worth noting, this will make development builds of ember run slower (but will leave prod builds the same). Unfortunately short of either taking a drastically different approach, or disabling mandatory setter entirely, I'm unsure of another approach.

It is possible for us to explore micro-related improvements, but...

[BUGFIX beta] allow watching of ES5+ Getter

Following this TODO: emberjs#12491 (comment)

stefanpenner force-pushed the fix-es5-watched-getter branch from 23d42e3 to 7b5de09 Compare October 16, 2015 23:04

stefanpenner force-pushed the fix-es5-watched-getter branch from 7b5de09 to 0b8814f Compare October 17, 2015 00:01

stefanpenner changed the title ~~[BUGFIX Release] allow watching of ES5+ Getter~~ [BUGFIX beta] allow watching of ES5+ Getter Oct 17, 2015

stefanpenner force-pushed the fix-es5-watched-getter branch 2 times, most recently from e7c98e5 to 5c81e5e Compare October 18, 2015 15:00

stefanpenner mentioned this pull request Oct 18, 2015

[BUGFIX Beta] unwatch should not create a new meta #12492

Closed

1 task

rwjblue reviewed Oct 18, 2015
View reviewed changes

stefanpenner force-pushed the fix-es5-watched-getter branch from 5c81e5e to 48b5298 Compare October 18, 2015 22:04

stefanpenner force-pushed the fix-es5-watched-getter branch from 48b5298 to 7daec5c Compare January 7, 2016 00:46

stefanpenner added 4 commits January 6, 2016 16:51

[BUGFIX beta] unwatch should not destroy foreign descriptor

a0d7dec

[BUGFIX beta] Ember.set should not smash ES5 descriptor

aed80aa

[fixes 12480]

[BUGFIX beta] remove redundant code (already the default M getter)

68d262a

stefanpenner force-pushed the fix-es5-watched-getter branch 5 times, most recently from f12c519 to 3a522ca Compare January 10, 2016 23:53

stefanpenner force-pushed the fix-es5-watched-getter branch from 3a522ca to 4c048be Compare January 10, 2016 23:54

[BUGFIX beta] MandatorySetter should work with inheritance.

4e052ff

stefanpenner force-pushed the fix-es5-watched-getter branch from 4c048be to 4e052ff Compare January 11, 2016 18:40

stefanpenner mentioned this pull request Jan 15, 2016

Setting a previously undefined but observed property in init causes it to be inaccessible via get #12817

Closed

stefanpenner added a commit that referenced this pull request Jan 21, 2016

Merge pull request #12491 from stefanpenner/fix-es5-watched-getter

2e7991e

[BUGFIX beta] allow watching of ES5+ Getter

stefanpenner merged commit 2e7991e into emberjs:master Jan 21, 2016

stefanpenner deleted the fix-es5-watched-getter branch January 21, 2016 19:17

homu mentioned this pull request Jan 21, 2016

WIP Restore meta-descs #12802

Closed

6 tasks

tricknotes mentioned this pull request Feb 2, 2016

Ember.set clobbers ES5 Computed property setters #12480

Closed

tricknotes added a commit to tricknotes/ember.js that referenced this pull request Feb 2, 2016

[Cleanup beta] Move test for Ember.set to observer_test

0fe1790

Following this TODO: emberjs#12491 (comment)

tricknotes mentioned this pull request Feb 2, 2016

[Cleanup beta] Move test for Ember.set to observer_test #12894

Merged

This was referenced Feb 4, 2016

Weird each regression in 2.3.0 #12906

Closed

ES5 getters on an Ember.Object only ever return first computed value #12940

Closed

stefanpenner mentioned this pull request Feb 12, 2016

cannot alway set a new value to a oneWay property #12953

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BUGFIX beta] allow watching of ES5+ Getter #12491

[BUGFIX beta] allow watching of ES5+ Getter #12491

stefanpenner commented Oct 16, 2015

rwjblue commented Oct 16, 2015

stefanpenner commented Oct 16, 2015

rwjblue commented Oct 16, 2015

stefanpenner commented Oct 16, 2015

stefanpenner commented Oct 17, 2015

bmac commented Oct 17, 2015

stefanpenner commented Oct 18, 2015

stefanpenner commented Oct 18, 2015

mixonic commented Oct 18, 2015

stefanpenner commented Oct 18, 2015

rwjblue Oct 18, 2015

stefanpenner Oct 18, 2015

stefanpenner Oct 18, 2015

stefanpenner Oct 18, 2015

stefanpenner Oct 18, 2015

stefanpenner commented Oct 24, 2015

stefanpenner commented Jan 11, 2016

stefanpenner commented Jan 11, 2016

stefanpenner commented Jan 11, 2016

stefanpenner commented Jan 11, 2016

stefanpenner commented Jan 20, 2016

[BUGFIX beta] allow watching of ES5+ Getter #12491

[BUGFIX beta] allow watching of ES5+ Getter #12491

Conversation

stefanpenner commented Oct 16, 2015

rwjblue commented Oct 16, 2015

stefanpenner commented Oct 16, 2015

rwjblue commented Oct 16, 2015

stefanpenner commented Oct 16, 2015

stefanpenner commented Oct 17, 2015

bmac commented Oct 17, 2015

stefanpenner commented Oct 18, 2015

stefanpenner commented Oct 18, 2015

mixonic commented Oct 18, 2015

stefanpenner commented Oct 18, 2015

rwjblue Oct 18, 2015

Choose a reason for hiding this comment

stefanpenner Oct 18, 2015

Choose a reason for hiding this comment

stefanpenner Oct 18, 2015

Choose a reason for hiding this comment

stefanpenner Oct 18, 2015

Choose a reason for hiding this comment

stefanpenner Oct 18, 2015

Choose a reason for hiding this comment

stefanpenner commented Oct 24, 2015

stefanpenner commented Jan 11, 2016

stefanpenner commented Jan 11, 2016

stefanpenner commented Jan 11, 2016

stefanpenner commented Jan 11, 2016

stefanpenner commented Jan 20, 2016