[BUGFIX beta] Adds a deprecation message when using targetObject

[sduquej]

For #14168

@rwjblue is this enough or should we also assert when setting targetObject? If so, where might I add that check?

[rwjblue]

This is good for the specifying of a targetObject, but we also need to deprecate folks asking for it (i.e. this.get('targetObject')).

The thought was that we would need to use an actual JS getter with a deprecation for that. Something like:

import { descriptor }  from 'ember-metal';

// ...snip...

 targetObject: descriptor({
    get() {
      deprecate('good message', false, { until: '2.12.0', url: 'some-path', id: 'ember-views.targetObject' });
      return this._targetObject;
    },

    set(value) {
      deprecate('good message', false, { until: '2.12.0', url: 'some-path', id: 'ember-views.targetObject' });
      return this._targetObject = value;
    }
  })

[sduquej]

Thanks, @rwjblue. That was really helpful.

[pixelhandler]

@sduquej Can you add the code (provided by @rwjblue) to define targetObject as deprecated via set/set to the mixin as well ?

• https://github.com/emberjs/ember.js/blob/master/packages/ember-runtime/lib/mixins/target_action_support.js ?

[sduquej]

Tried that @pixelhandler, but then ran into tests failing as classes that include the Ember.TargetActionSupport mixin would now have a targetObject property (the descriptor).

There's a few places where the framework iterates over all properties in an object so targetObject would come up raising the deprecation warning, for example here: https://github.com/emberjs/ember.js/blob/master/packages/ember-runtime/lib/inject.js#L56

It didn't feel right to add special-case handling in these places as they were a consequence of adding the code to the mixin which is why we came up with the init approach.

[rwjblue]

Ya, we have to make it non-enumerable (which will cause it to not show up in list of keys).

[sduquej]

I totally missed that option 😅. Have rebased and udpated the PR accordingly.

[sduquej]

What's the best way to silence deprecations within the framework itself?
I read https://github.com/emberjs/rfcs/blob/master/text/0065-deprecation-warning-handlers.md
but found no usages outside of ember-debug.

The build is currently failing in the production suite because I want to silence deprecations here:
https://github.com/emberjs/ember.js/pull/14590/files#diff-eba568646bdc7cb8b35def52b7e49e9cR173

[MiguelMadero]

Is the idea to use target instead?

In Ember 2.7 we used to be able to read and set targetObject. It's gone in 2.9, but using _targetObject worked. While both are private they worked. By reading this, if I understood correctly the idea is to use target instead, but that didn't work in the way I was using it on adopted-ember-addons/ember-cli-hot-loader#34

We have inside a component wrapper something like:

{{#component wrappedComponentName _targetObject=_targetObject 

I understand based on the deprecation warning that we were supposed to use target instead, however that didn't work either

{{#component wrappedComponentName target=target 

What worked across versions was, but that's supposed to be deprecated is reading from _targetObject and setting targetObject:

{{#component wrappedComponentName targetObject=_targetObject 

[rwjblue]

I think we should remove this line and the line below that does target = get(instance, 'targetObject') (so that we don't trigger the deprecation).

Things should fall through now that we have the getter/setter above.

[sduquej]

👍 Updated

[sduquej]

Changed this to 2.15.0. Perhaps we'd like it to be a later version, if so which?

[rwjblue]

Ya, it will need to be 2.17.0 at this point.

[sduquej]

@rwjblue Thanks! Updated.

[GavinJoyce]

@rwjblue it would be good to get this merged. As it's been ~6 weeks since the deprecation target was set to 2.17.0, perhaps it now needs to be 2.18.0?

[rwjblue]

Unfortunately, I think we actually want to duplicate the code for this helper method so that it can properly be stripped from production builds.

Can you move the code into both the get and set directly?

[rwjblue]

I think we should pass a bit more debugging info here. If I saw this message in the console, I'm not sure how I would track down where it was coming from. Perhaps you can include this (which will display something like <component:foo-bar#1234> which might give someone a clue...).

[rwjblue]

I've let this slip yet again, and I am very sorry. Can you update this again to 3.5.0?

[sduquej]

@rwjblue Updated! 🙇

[rwjblue]

Thank you very much for your patience and hard work here. Sorry for all the delays, great work! 👏👏🎉

[sduquej]

🍻 @rwjblue No problem at all! Perhaps you'd like to close the original issue (#14168) now?