Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

possibleConstructorReturn helper fails to return correct constructor when extending native type #6918

Closed
anyuzer opened this issue Nov 27, 2017 · 2 comments
Labels
i: duplicate outdated A closed issue/PR that is archived due to age. Recommended to make a new issue

Comments

@anyuzer
Copy link

anyuzer commented Nov 27, 2017

Bug Report:
When extending a native type possibleConstructorReturn helper returns the root prototype, instead of the correctly extended prototype.

Input Code

class ObjX extends Object{
  logKeys(){ 
    console.log(Object.keys(this)); 
  }
}

const exObject = new ObjX;
exObject['key'] = 'val';
exObject.logKeys();

Babel/Babylon Configuration (.babelrc, package.json, cli command)

{
  "presets":["es2015","stage-1"]
}

Expected Behavior

If you drop the above code directly in your browser console, it will quite happily output an array of ['key'].

Extending native types, while uncommon has been a standard capability in javascript for a really long time. This behavior should be preserved by babel.

Current Behavior

Currently, Babel will just bind the native constructor to the extended class. So calling new ObjX results in just a native object (none of the methods or properties of the extended class are preserved).

Possible Solution

I spent an hour or two digging through Babel core and the flaw in logic is pretty apparent. When you extend a native class, Babel does this:

return _possibleConstructorReturn(this, (ObjX.__proto__ || Object.getPrototypeOf(ObjX)).apply(this, arguments));

With possibleConstructorReturn relying on the following:

return call && (typeof call === "object" || typeof call === "function") ? call : self;

In the case of creating a new class in Babel, the origin class doesn't call possibleConstructorReturn, it just does the classCallCheck. When you extend that, possibleConstructorReturn is called on a class that extends a brand new prototype. In which case, it does its error check and returns nothing, you end up with the 2 arguments, one being the extended prototype, and one being undefined. All works as it should.

In the case of extending a native type, by calling apply on the prototype you end up with a newly assigned value, which leaves us with self and call defined in possibleConstructorReturn and the blank native gets returned instead of self, which gives you the native prototype instead of your extended class.

It's pretty easy to solve this in a small scope, because when extending a native type it should just return self (in which case everything works fine, in this limited scenario). I was able to play with the logic in possibleConstructorReturn to try to get it to both work, as well as pass all of the current test scenarios, but when I tried it out in practice it obviously impacted other things.

This tells me that the logic that currently exists is very purposeful, but is also tightly coupled to handle a lot of scenarios, and it felt like touching this code required somebody more familiar with how/why Babel does classes the way it does.

Perhaps a low impact solution would be when generating the code, check to see if the class is extending a native type, in which case skip the possibleConstructorReturn logic when compiling the class?

@babel-bot
Copy link
Collaborator

Hey @anyuzer! We really appreciate you taking the time to report an issue. The collaborators
on this project attempt to help as many people as possible, but we're a limited number of volunteers,
so it's possible this won't be addressed swiftly.

If you need any help, or just have general Babel or JavaScript questions, we have a vibrant Slack
community that typically always has someone willing to help. You can sign-up here
for an invite.

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Nov 27, 2017

Hi, I'm closing this as a duplicate of #4480.
There is also a proposed solution (TL;DR: https://github.com/WebReflection/babel-plugin-transform-builtin-classes)

@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label May 3, 2018
@lock lock bot locked as resolved and limited conversation to collaborators May 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
i: duplicate outdated A closed issue/PR that is archived due to age. Recommended to make a new issue
Projects
None yet
Development

No branches or pull requests

4 participants