[WIP] [BUGFIX release] Ensure CP's are _super wrapped.

[rwjblue]

Prior to this change, a CP that was not included in the superclass would not get re-wrapped. This means that when calling it from another function that is required to be super wrapped would set that other function as the _super.

This changes to ensure that all descriptors are wrapped if they include _super (hasSuper check on platforms that support Function.prototype.toString).

Fixes #13230.

/cc @stefanpenner

[stefanpenner]

Lgtm, @krisselden r?

[mixonic]

This seems reasonable.

[mixonic]

wrapping

[rwjblue]

Good catch! Updated...

[netes]

👍 for this one

[rwjblue]

This also was an issue for normal methods (aka non-CP's). I fixed that and added an additional test for it.

[rwjblue]

OK, so forcing wrapping of all functions that match the hasSuper check causes issues for any classes on the object.

The following breaks:

export default Ember.Object.extend({
  baseModel: Ember.Object.extend()
});

One specific example is glimmer templates since they are now a class (and get instantiated) by the container, where you would have Ember.Component.extend({ layout: compile('foo') }) (and layout would get super wrapped causing it to loose the .create / .extend methods on the class).

[rwjblue]

I can remove the last commit and land the fix for CP's (which seems less of a compat concern), but we need to decide what to do about the test added for normal method's:

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

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

  foo() {
    return this._super(...arguments) + ' | B - Foo | ' + this.bar();
  }
});
let b = B.create()
b.foo();

// => "A - Foo | B - Foo | A - Foo| B - Bar"

When B.prototype.bar calls this._super() it gets A.prototype.foo...

Runnable Demo

[krisselden]

you should use the ROOT that wrap uses for this, so it is marked as not having super explicitly

[krisselden]

It is non obvious to me that you should be able to call super without having a method you are overriding. I know we handled a case like this for mixins, after talking with @wycats this should have just been an error. I can't think of another language where you can call super without actually overriding anything.

Also, the fix to hasSuper for CP macros I think was wrong, and I'm nervous about how many things will be wrapped by this PR, it seems like instead of the REGEX being loosened so much, that the macros themselves should have likely just passed generatedFn.hasSuper = hasSuper(callback) anyway, I think at a minimum we should consider changing the auto terminal super behavior to be deprecated and warn in this case.

[rwjblue]

@krisselden - Thanks for reviewing!

It is non obvious to me that you should be able to call super without having a method you are overriding.

If that is true we should error not silently ignore/swallow the this._super call.

Also, the fix to hasSuper for CP macros I think was wrong

Which fix?

I think at a minimum we should consider changing the auto terminal super behavior to be deprecated and warn in this case.

I'm not certain what exactly this means, but we can discuss the first point on the next core team call to figure out a path forward.

[krisselden]

@rwjblue it means the original test case that @mixonic did for Mixin.create() calling a _super and being mixed into something that did not have that, that was covering the same issue but when it was set to itself so it infinitely looped.

[krisselden]

@rwjblue the fix I was referring to with CPs was loosening the REGEX for hasSuper to _super or invoking something that could call super. Anyway, that loosening is what makes it difficult to fix the above case without hurting props that aren't methods, but even then, tightening it wouldn't fix it if toString() wasn't present. So I guess it doesn't matter. Props that are functions where we don't want them wrapped is a problem that our syntax doesn't allow us to really disambiguate.

[rwjblue]

We had a long discussion about this during the last core team meeting (see notes here). The basic conclusions are:

• 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.
• We should ensure that all framework hooks have a default implementation present (to avoid the error mentioned above).

Will continue to follow up with @krisselden and @wycats to figure out the right path forward for this...

[offirgolan]

Thank you for taking the time to figure this out fellas. As of right now, is there a current workaround to check if a CP has a super?

[rwjblue]

@offirgolan - Not really. I suppose you could do something like this, but it pretty much sucks.

[offirgolan]

@rwjblue Yeah I see what you mean. I guess it could be an intermediate solution but if you are going to deprecate calling _super when the parent class doesn't have the method defined, I feel like there should be a helper method exposed to check that.

[rwjblue]

@offirgolan - I think we will likely warn/deprecate/something. As @krisselden mentioned, most programming languages do not allow calling super when there is no super, so I suppose this is basically the same thing...

[offirgolan]

@rwjblue Ah okay totally understandable. Thanks for the clarification 😄

[rwjblue]

Closing in favor of #14780.