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

Class properties should be copied over when inheriting #2450

Closed
sophiebits opened this issue Sep 29, 2015 · 13 comments
Closed

Class properties should be copied over when inheriting #2450

sophiebits opened this issue Sep 29, 2015 · 13 comments
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue

Comments

@sophiebits
Copy link
Contributor

This code logs 5 in most browsers, but B.foo is undefined in IE10 and older:

class A {}
A.foo = 5;

class B extends A {}
console.log(B.foo);

This happens because core-js doesn't define Object.setPrototypeOf if the browser doesn't support __proto__ so this line does effectively nothing:

if (superClass) Object.setPrototypeOf ? Object.setPrototypeOf(subClass, superClass) : subClass.__proto__ = superClass;
.

Perhaps the static properties could be copied instead? That's what jstransform did.

cc @sebmarkbage

@sebmck
Copy link
Contributor

sebmck commented Sep 29, 2015

You can either override Object.setPrototypeOf to do your shallow copying or use babel-plugin-object-set-prototype-of-to-assign.

@sophiebits
Copy link
Contributor Author

Thanks. Do you not think this is a useful default?

@sebmck
Copy link
Contributor

sebmck commented Sep 29, 2015

As in, copy the properties by default?

@sophiebits
Copy link
Contributor Author

At least if __proto__ isn't supported.

(And maybe in loose mode even when it is? Supposedly setting prototypes is slow. I haven't been following Babel 6 so I don't know if you even have something called "loose mode" any more.)

@sebmarkbage
Copy link

Not convinced that implementing your own Object.setPrototypeOf is the best story here since that can be used as a feature test and assumptions made that it will respect mutation of the prototype and hasOwnProperty. It also applies to all objects. Limiting this compliance violation to classes seems like the best compromise to me.

If it changes behavior between IE10 and other browsers, you'll end up with weird bugs. Personally, I prefer being consistent than compliant for production code. IE10 is still a very much alive unfortunately. However, the perf argument is probably even better since that affects a lot more users.

@sebmck
Copy link
Contributor

sebmck commented Sep 29, 2015

Is there a usecase that the plugin I linked doesn't cover? It will replace Object.setPrototypeOf with a function that copies all properties from the super class.

@sophiebits
Copy link
Contributor Author

(As a little more context: We were planning to set React.Component.isReactClass and then check Foo.isReactClass to see if Foo is a React component. This doesn't work in IE10. We're probably going to set a flag on the prototype instead because of compat with other environments (e.g., scala-js) but it would be nice if this Just Worked™ in Babel.)

@sophiebits
Copy link
Contributor Author

Using that plugin means you can't feature-test for and use Object.setPrototypeOf in your own code, doesn't it? @sebmarkbage suggested doing the transformation only for classes (only for the inherits helper).

@lukescott
Copy link
Contributor

There is also babel-plugin-proto-to-assign.

I'm somewhat torn on this. I understand the desire for it to "just work". Most of us have to support IE 10 - there just isn't an option. The solutions mentioned adds a lot of friction. Especially for @spicyj and his team who then have to educate React developers who use babel to properly transform their helpers.

Copying breaks inheritance. So it shouldn't be "always-on" as that would break code in other platforms. However, falling back to copying might not be such a bad thing, if developers are made aware of the caveats.

One way to handle this is by throwing warnings when static properties are used, which link to the caveats of using static properties in IE 10 and below. That could be done using an on by default plugin. To silence the warnings you could either blacklist that plugin or provide a comment annotation where the warnings occur.

@sebmck
Copy link
Contributor

sebmck commented Oct 5, 2015

@spicyj

Using that plugin means you can't feature-test for and use Object.setPrototypeOf in your own code, doesn't it?

Nope. I'm not sure if that's an issue since you're forgoing proper semantics anyway in favor of your copy method

@sebmarkbage suggested doing the transformation only for classes (only for the inherits helper).

I don't think that having different semantics for static inheritance is a very good idea. Especially when you can achieve what you want reliably and across all possible environments through other methods.

@SimenB
Copy link
Contributor

SimenB commented Oct 7, 2015

Quick question, is babel-plugin-proto-to-assign always a plugin? Doing like it says here (putting it in optionals) doesn't seem to work. I can't get IE <= 10 to work anyway, but at least my console.logs gets triggered when added to plugins, but not in optional

@mairatma
Copy link

I'm having a similar problem with IE <= 10, but worse. It seems like I can't call parent constructors anymore on babel 6 because the call gets transpiled to babelHelpers.possibleConstructorReturn(this, Object.getPrototypeOf(Component).call(this, opt_config));, which doesn't work on IE <= 10 because of the Object.setPrototypeOf call inside inherits that you've been discussing here.

I've tried using babel-plugin-transform-object-set-prototype-of-to-assign, but it didn't work. I think it's because the Object.setPrototypeOfcall to be replaced is inside the inherits helper, so maybe it's not affected by the plugin or it doesn't work due to plugin order. Is there a way around this?

I'm commenting on this issue instead of creating another one since it's due to the same cause, but it's a different consequence.

@webcarrot
Copy link

@mairatma
try proto-polyfill - without object-set-prototype-of-to-assign and loose mode.

@jridgewell jridgewell reopened this Sep 8, 2017
imcuttle added a commit to be-fe/react-mobx-vm that referenced this issue Apr 16, 2018
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label May 4, 2018
@lock lock bot locked as resolved and limited conversation to collaborators May 4, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
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

8 participants