DEFAULT_GETTER_FUNCTION returns wrong value if actual value is set to undefined

[bastimeyer]

Hi!
While I was trying to upgrade my app by using ember-decorators, I encountered an issue with Ember's DEFAULT_GETTER_FUNCTION, Meta#_findInherited2 to be precise. The issue is unrelated to the decorators themselves, but I noticed it because of the observes decorator, which is using Ember.addObserver instead of Ember.observer and changing my app code seemed like a trivial thing to do.

It looks like this issue is present in all recent Ember releases.

Here's an ember-twiddle which demonstrates the problem:
https://ember-twiddle.com/8c7201ea559a9591219ea58edce56d52?openFiles=tests.foo-test.js%2C

---

I don't know much about the internals of Ember's object system, but the core of this issue is that you can't set a regular property to undefined if it has an observer attached via the Ember.observer method. Observers added via Ember.addObserver are working fine, though.

The reason for this is the difference of the two getter methods, DEFAULT_GETTER_FUNCTION and INHERITING_GETTER_FUNCTION, and the Meta methods which they are using:
https://github.com/emberjs/ember.js/blob/v3.4.4/packages/ember-metal/lib/properties.ts#L72-L100

DEFAULT_GETTER_FUNCTION, which seems to be the getter for observed regular properties if Ember.observer is being used, is calling Meta#peekValues, which itself is calling Meta#_findInherited2. As you can see in this method, the meta-object's value will only be returned if it's not undefined, otherwise the value of one of the parent meta-objects will be returned. This means that if you set the property to undefined, it'll return its parent value on the next lookup.

INHERITING_GETTER_FUNCTION on the other hand, which seems to be used as the getter for regular properties which are observed via Ember.addObserver, is calling Meta#readInheritedValue, which is very similar to Meta#_findInherited2. However, here, it is also being checked whether a value exists on the meta-object via subkey in map and an UNDEFINED symbol will be returned if it doesn't exist. This means that undefined values are taken into account, which is the proper thing to do.

On a side note, wouldn't it make sense using hasOwnProperty here instead of checking for existing properties with in, even if it's just a plain object, as it is much faster, and also removing the first part of the if-statement's disjunction, as it is redundant?

Like I've already said, I'm unfamiliar with how Ember works under the hood, but why are there two similar methods being used for two different getter methods?

Thanks!

[rwjblue]

Thanks for taking the time to dig into this and document what is going on so thoroughly!

This definitely seems like a bug that we should address.

---

On a side note, wouldn't it make sense using hasOwnProperty here instead of checking for existing properties with in, even if it's just a plain object, as it is much faster, and also removing the first part of the if-statement's disjunction, as it is redundant?

I do not believe that hasOwnProperty is actually faster in this circumstance, but please feel free to test it out and see.

[bastimeyer]

I do not believe that hasOwnProperty is actually faster in this circumstance

You're right, just tested it again. The test suite I had used earlier wasn't ideal.