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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix class inheritance in IE10 #7969

Merged
merged 8 commits into from May 23, 2018
Merged

Conversation

jridgewell
Copy link
Member

This reverts commit f8ab946, and unfixes #7771, because of IE 10. 馃槱

Also updates the getPrototypeOf helper according to #7965 (comment).

Fixes #7892.
Fixes #7965.

@jridgewell jridgewell added the PR: Bug Fix 馃悰 A type of pull request used for our changelog categories label May 18, 2018
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some fixtures need to be updated

subClass.prototype = Object.create(superClass && superClass.prototype, {
constructor: {
value: subClass,
enumerable: false,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could be better to omit enumerable: false just to not force sham implementations to throw, but this would only happen on IE<9 anyway.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the spec inherits. I think changing it to be enumerable would break more people who use a for-in.

You can always use loose mode to support these IE9?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think they just mean that the default is already false anyway, so leaving it out doesn't make anything fail, but allows polyfills to be less agressive in requiring enumerability on older platforms.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, ok.


test() {
}
Object.defineProperty(Obj.prototype, 'test', {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand what caused this change. Any idea? It doesn't seem like this should be split off like this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the reason I filled #7771. jridgewell@ab403f3#diff-0cd85dd4813df0687b8938547953d585L24

test here will be set on the subclass, but the super defines a get test, so it'll throw.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh got it.

};
_getPrototypeOf = Object.setPrototypeOf
? Object.getPrototypeOf
: function _getPrototypeOf(o) { return o.__proto__; };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'd still want to fall back to Object.getPrototypeOf if __proto__ falsy, since that was the old behavior.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@babel-bot
Copy link
Collaborator

babel-bot commented May 18, 2018

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/8054/

_getPrototypeOf = Object.getPrototypeOf;
} else {
_getPrototypeOf = function _getPrototypeOf(o) {
return o.__proto__;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think my comment here was misinterpreted. My concern was that this does not preserve the behavior changed in https://github.com/babel/babel/pull/7675/files#diff-a6fddd06921a2b0da9eb1fb68cd794b9L14

Old super calls used to be (Foo.__proto__ || Object.getPrototypeOf(Foo)).call, but here we assume that __proto__ is the source of truth and will always have a value, instead of falling back to Object.getPrototypeOf of __proto__ is falsy.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, that was dyslexic of me. Fixed.

@hzoo hzoo merged commit 2af7a33 into babel:master May 23, 2018
@benjamn
Copy link
Contributor

benjamn commented May 23, 2018

How soon can this be released? Meteor implemented a pretty nasty workaround for this, which I would love to revert before shipping Meteor 1.7. I would have reported the problem, but it looked like Babel had simply decided to drop support for IE10, so I didn't think you'd be open to it.

@loganfsmyth
Copy link
Member

@benjamn Likely tomorrow. What's your timetable for 1.7?

@benjamn
Copy link
Contributor

benjamn commented May 23, 2018

Tomorrow is great. We're on RC 11 right now, but I think this is a good reason to wait a little longer! I'm giving a talk at Meteor Night next Wednesday, with a call-to-action to update to Meteor 1.7, so we'd better release Meteor 1.7 before then, but I suppose we can just ship the workaround if the next Babel beta somehow doesn't get released by then.

@jridgewell jridgewell deleted the class-inheritance branch May 24, 2018 15:04
benjamn added a commit to meteor/meteor that referenced this pull request May 25, 2018
This reverts commit 8bfdea7.

As clever as this IE10 hack may have been, it should no longer be
necessary, thanks to babel/babel#7969.

cc @abernix @hwillson
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 4, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 4, 2019
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 PR: Bug Fix 馃悰 A type of pull request used for our changelog categories
Projects
None yet
7 participants