Further refine the error messages around allowing normal JavaScript property gets

[wycats]

This issue is tracking problems that come up due to allowing normal JavaScript property lookup to work with computed properties.

Context

We are in the process of landing a change that will allow you to use normal property lookups to get properties defined as computed properties in Ember.

In practice, what this means is that, in most cases (more later) you will be able to drop .get('key') and replace it with .key.

However, there are a handful of small historical issues that could make the transition a little more messy. If you landed on this issue, chances are you encountered a warning from Ember that you encountered one of these historical issues.

Computed Properties used to be directly on the prototype

Until Ember 3.0, internal implementation details of Ember caused computed properties to be directly present on objects, so if you didn't use .get(), you would see the internal "Ember descriptor" representing the computed property (a private API).

This internal implementation detail was never intended to be a public (or even intimate) API.

This internal implementation detail has now changed and the (still private) "descriptor" object has been relocated to the object's "meta" (also a private API). This allows us to make normal property lookup work, and in general, very few people were relying on the presence of this internal descriptor.

If you encountered an error related to this problem, you are probably using an addon that relies on this now-defunct private implementation detail, and your best bet is to follow the instructions in the error message (and ask for help in #dev-ember).

Ember Proxies are tricky

Secondly, since the new behavior relies on using defineProperty, it does not allow users of Ember proxies to stop using .get().

Previously, since all Ember objects required you to use .get() to access properties, you could uniformly use .get() in your code and never need to worry about whether an object was an Ember proxy.

However, now that you can use normal property access for computed properties, Ember proxies are special. If an object is implemented using an Ember proxy, it should advertise itself as requiring .get() to access its proxied properties.

This is analogous to other libraries in the JavaScript ecosystem, such as immutable.js, that expose an API that is different from normal JS property access patterns.

Since proxies are relatively rare in Ember compared to property access on other Ember objects, emberjs/rfcs#281 (now merged) felt that the cost of losing consistent property access (everyone types .get() everywhere, so you don't need to know when an object is "special") was worth the benefit of normally not needing to use .get().

During the transition, we are worried that people will accidentally migrate some .get()s on Ember proxies to regular property access, so we added an assertion to the code that fires if:

• you use a normal property get on an Ember proxy
• the property is not present on the object itself
• unknownProperty returns a value other than undefined

(this development-mode assertion is implemented using ES2015 proxies, which are not yet present on all supported browsers, but will cover the browsers people typically use for development)

For the most part, this avoids the vast majority of mistakes but it does introduce a possible problem with code that "probes" objects to discover whether they implement protocols.

Here's what I mean by "probing" code:

if (obj.toJSON) {
  return obj.toJSON();
}

A more resilient way to write this code would be:

if ('toJSON' in obj && typeof obj.toJSON === 'function') {
  return obj.toJSON();
}

Unfortunately, apps are not always in control of the probing code (which could come from test frameworks, generic serialization logic, etc.).

Notably, "probing" code is only a problem if:

• an Ember proxy finds its way to the probing code
• the property is not present on the object itself (not true about toString, which is usually a real property of the Ember proxy itself)
• and this is the important part: unknownProperty returns something other than undefined

It's definitely possible that an Ember proxy could return some object for every possible property, and that some probing code checks for a protocol that's not usually present on the wrapper object (like toString), which would trigger the assertion inappropriately.

This problem comes up now because probing code previously would only look at properties of the wrapper proxy object, but now, in order to provide good errors for people accidentally removing .get() that they shouldn't have, normal property accesses on Ember proxies become sensitive to the behavior of unknownProperty.

Again, the correct mental model going forward is that these objects (most notably the values returned from relationships in Ember Data) are now special objects and require a non-standard API, but the process of migration introduces this ambiguity.

If you got this error and didn't expect it, please reply to this issue with information about what happened so we have help improve the error message with more further advice and workarounds.

[mani-mishra]

An example of the non resilient probing code is JSON.stringify implementation, which I encountered at #16323

[Dhaulagiri]

I'm seeing this error get triggered by code we don't control in chai.js

// Chai.js code

if (value && typeof value.inspect === 'function' &&

// Error

You attempted to access the `inspect` property (of <DS.Errors:ember460>)

This is the line of test code that triggers this, where component.get('visible') in this case is an array of ember data records returned from a computed.uniq filter:

(0, _chai.expect)(component.get('visible')).to.contain(emberDataRecord);

[rwjblue]

@Dhaulagiri - That specific issue should have been addressed in emberjs/data#5340 (which was released in ember-data@3.1.0).

However, I do still believe there are some changes that we could make to reduce (and avoid the DS.Errors like issue). IMHO we should update the assertion system to allow null or undefined here:

ember.js/packages/ember-runtime/lib/system/core_object.js

Lines 128 to 130 in 2c1f9bf

	let value = target.unknownProperty.call(receiver, property);
	assert(messageFor(receiver, property), value === undefined);

[Dhaulagiri]

@rwjblue sure enough, using ED 3.1 seems to fix it. Sadly we have been trapped on ED 2.12 for a long time so that fix won't be of immediate help to us as we try to upgrade ember itself, so I may explore your second suggestion.

[rwjblue]

@Dhaulagiri - FWIW, many/most of the ember-data upgrade blocking issues that happened in the 2.12 era have been addressed in ember-data@3.2.0-beta.2. Might be worth a try...

[denzo]

@cibernox selected.then throws an actual error in Ember 3.1.0

https://github.com/cibernox/ember-power-select/blob/master/addon/components/power-select.js#L141

[rwjblue]

@denzo - Seems like a bug report should be filed in ember-power-select, no?

[huberts]

@denzo - Have you reported this as a bug in ember-power-select? Maybe @cibernox is not aware of the problem. I have the same issue wtih power select in my app.

[cibernox]

@huberts I was not. What is wrong exactly with typeof selected.then === 'function'? Seems like a pretty reasonable pattern. What is a safe way of checking if an object is a thenable?

[cibernox]

@huberts @denzo The bug should be fixed in 2.0.0-beta.4 of ember-power-select.

[rwjblue]

There is an open PR in the works (over in #16560) to prevent the proxy assertion from applying to functions. This fixes a large number of the existing reported issues (e.g. the ember-power-select probing for selected.then), and still provides nice guidance for folks that may accidentally attempt to remove .get when retrieving CP values on a proxy.

[akshaisarma]

Just to clarify: with this change, are we still supposed to be able to use ember-metal functions on Proxy objects? I am getting this assertion triggered in testing when I call isEmpty on a Proxy object:

https://github.com/emberjs/ember.js/blob/master/packages/ember-metal/lib/is_empty.js#L45

  if (typeof obj.size === 'number') {
    return !obj.size;
  }

[pixelhandler]

@wycats do you know if any effort has been done to further refine the errors?

Sounds like we need to provide a good warning message for the scenario where dot access was attempted for a proxy object ?

Since Ember 3.1, this is usually fine as you no longer need to use .get() to access computed properties. However, in this case, the object in question is a special kind of Ember object (a proxy). Therefore, it is still necessary to use .get('length') in this case.

Also need to be sure this is well understood ^

Thanks @yininge for pointing out we need to revisit this one :)

Perhaps instead of You attempted to access the inspect property (of <DS.Errors:ember460>) we should identify that a proxy object was involved and explain that .get() is required ?

Per the 3.1 blog post we have instruction on when to use .get().

If you are calling get with a chained path. For example in this.get('a.b.c') if b is undefined the return value is undefined. Converting this to this.a.b.c when b is undefined would instead raise an exception.

If your object is using unknownProperty you must continue to use get. Using an ES5 getter on an object with unknownProperty will cause an assertion failure in development.

Ember Data returns promise proxy objects when you read an async relationship and from other API. Ember proxy objects, including promise proxies, still require that you call get to read values.

My best guess is that the last item for a proxy describes what needs more detail in an error message.

[rwjblue]

Closing this for now. Happy to reopen, but these messages have been around for quite a long time now and we aren't working on changing them.