Ember.js: Deprecate calling `this._super` when there is no method on parent class.

[rwjblue]

We should not support calling _super when the parent class does not have a method defined (aka when there is no _super). This likely means adding a deprecation for now, and converting that to an error in 3.0.0.

Examples:

let A = Ember.Object.extend({
  foo() { return 'A - Foo'; },

  qux: Ember.computed(function() {
    return 'A - qux';
  })
});

let B = A.extend({
  bar() {
    // should trigger deprecation
    return this._super(...arguments) + '| B - Bar';
  },

  foo() {
    // should not trigger deprecation
    return this._super(...arguments) + ' | B - Foo | ' + this.bar();
  },

  qux: Ember.computed(function() {
    // should not trigger deprecation
    return this._super(...arguments) + ' | B - qux';
  }),

  baz: Ember.computed(function() {
    // should trigger deprecation
    return this._super(...arguments) + ' | B - baz';
  })
});

See original context

[workmanw]

One thing that came to mind was the component lifecycle hooks (didReceiveAttrs, didUpdateAttrs, willRender, etc). It appears that this hooks are not defined as no-op functions on the component class ember-glimmer/lib/component.js.

It seems like this would trigger a deprecation:

export default Ember.Component.extend({
  didReceiveAttrs() {
    this._super();
    /* ... */
  }
});

That feels like it could be slightly problematic in that it requires each component class that extends another to know whether it's parent has implemented the lifecycle hook if it too wants to use that hook. And even then, it creates a bad practice in that you should always call _super on hooks.

Seems like one native solution would be to implement no-op functions on the Ember.Component class for each of these hooks. Thoughts? I could submit a PR if this seems favorable.

[rwjblue]

Yes, that is absolutely the same conclusion made in the comment that I linked to in the main issue description. PR's to ensure that all "framework hook" methods are properly defined are definitely welcome. 😺

[krisselden]

We should do that, but once again, the only issue that affects is mixins, and you can always define the base if the mixin expects the hook to be present. Otherwise, if you extend something you should understand the base class and what you are overriding.

[locks]

Otherwise, if you extend something you should understand the base class and what you are overriding.

It's not always easy in Ember ;P

[workmanw]

@krisselden Well what if I'm extending or mixing in from an addon. That addon doesn't currently implement didInsertElement so I don't call _super. But then a point release comes along and they do add a didInsertElement hook (or vice-versa they remove it and now my _super call throws). I have to regularly audit the code for all addons to be sure I'm not breaking anything.

[locks]

I have to regularly audit the code for all addons to be sure I'm not breaking anything.

This is a thing you're supposed to do.

[workmanw]

@locks You're saying you think all ember users should be regularly auditing the code of the addons they use every time there is a new point release? That seems impractical even for advanced developers.

[stefanpenner]

Otherwise, if you extend something you should understand the base class and what you are overriding.
It's not always easy in Ember ;P

more concisely it is not the easy in a language in an untyped dynamic language.

@locks You're saying you think all ember users should be regularly auditing the code of the addons they use every time there is a new point release? That seems impractical even for advanced developers.

@workmanw it is deprecation you must not audit every point release, rather every major. That being said the deprecation warning will surface future issues (for the next major) much sooner, allowing them to be addressed well ahead of being an actual blocker.

---

Deprecate calling this._super when there is no method on parent class.

This when combined with mixins (and current usage of mixins) may be a tricky transition. We will need to test the impact in the wild.

When comparing with other languages (and even JavaScript itself) it becomes rather obvious we need to tighten this up.

Although we may want to consider enabling detecting if something has a super.

As for example, ruby has defined?(super), JavaScript has if (super.a) { }, in Objective-c IIRC you could check via respondsToSelector

class Bar
end

class Foo < Bar
  def baz
     if defined?(super)
        # do super
       super
     end
     puts 'Foo.baz'
end

Foo.new.bar # => Foo.baz'

class Bar
  def baz
    puts 'Bar.baz'
  end
end

Foo.new.bar # =>  Bar.baz
# => Foo.baz

class Bar {
  a() {
     if (typeof super.a === 'function') {
       super.a();
    }
    console.log('Bar.a');
  }
}

new Bar.a() // => Bar.a
Object.prototype.a = function() { console.log('Object.a'); }
new Bar.a(); // => Object.a
// => Bar.a

[workmanw]

@stefanpenner I was just trying to point out that after Ember 3 when this does throw an error, it would be very easy for a change to an addon you're using create an error in your application, even in a point release for that addon. If the addon suddenly get's ride of didInsertElement and you were calling super, you'd now get an error (and vice-versa). Commonly deprecation errors for addons are because an addon was using a deprecated API. This is quickly fixed because it affects everyone equally. However with this issue, it would potentially affect user's uniquely because they, or the addon, may or may not be implementing a hook.

But it sounds like everyone is really agreeing on the value of implementing default hooks. 👍 .

[stefanpenner]

@stefanpenner I was just trying to point out that after Ember 3 when this does throw an error, it would be very easy for a change to an addon you're using create an error in your application, even in a point release for that addon. If the addon suddenly get's ride of didInsertElement and you were calling super, you'd now get an error (and vice-versa). Commonly deprecation errors for addons are because an addon was using a deprecated API. This is quickly fixed because it affects everyone equally. However with this issue, it would potentially affect user's uniquely because they, or the addon, may or may not be implementing a hook.

didInsertElement is or should be implemented in the base-class, this would not cause an issue.

[workmanw]

@stefanpenner Yea you're right about didInsertElement, that was my mistake. But this concern is still valid for didReceiveAttrs, didRender, willRender, and didUpdateAttrs.

[stefanpenner]

But this concern is still valid for didReceiveAttrs, didRender, willRender, and didUpdateAttrs.

Yup, we should be sure to add them to the base-classes if we take this route. That way this concern is also mitigated.

[givanse]

Trying to get this done here: https://github.com/emberjs/ember.js/pull/15082

[g13013]

@stefanpenner If I could suggest to turn it to a permanent warning instead of deprecation/error, this is because plugin authors may not play the game and release minor releases when removing hooks from base classes which could break actual apps, this could happen even after migration to ember 3.x.

[pixelhandler]

@givanse how you doing these days? Looks like we'd need to rebase #15082 @stefanpenner any idea if this still needs to be resolved?

[pixelhandler]

Per our triage policy I'll close this out for now, feel free to reopen if the issue persists on the current release.

• https://github.com/emberjs/ember.js/blob/master/CONTRIBUTING.md#triaging-policy

[rwjblue]

This is still an issue, and we should issue an error...