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

Remove dependence on ES5 shams per #4189 #4959

Merged
merged 1 commit into from
Oct 6, 2015
Merged

Conversation

dgreensp
Copy link
Contributor

React now only uses Object.create and Object.freeze in a couple places (one each, based on my grepping). This commit removes the dependency on having non-functional or incomplete shams for these methods in the global environment.

As previously discussed, use of Object.freeze merely needs to be guarded by a check for whether it exists. The discussed remedy for Object.create was to "use ES6 classes," but that doesn't seem in line with current style, and in any case it would be a larger change. The necessary code to perform Object.create(proto) with a non-null proto is three lines of code that are well-known to most developers. They could be inlined or broken out into a utility (which is what I chose here).

This code has not been tested in IE 8 yet; I wanted to start the ball rolling.

@facebook-github-bot
Copy link

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@@ -0,0 +1,33 @@
/**
Copy link
Contributor

Choose a reason for hiding this comment

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

The name is different, but this is effectively the same as Object.create, so this is just re-introducing the polyfill. We want to use ES6 classes instead as per #4189 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not a polyfill, because there is no accurate polyfill for Object.create; that's why we're in this predicament. :) The Object.create es5-sham (which I'm a contributor on) is over 100 lines. Setting a proto, on the other hand, is something every JS framework has to do at one point or another, since long before Object.create existed.

Can you elaborate on the ES6 class idea? Do you mean rewriting SyntheticEvent to use ES6 classes and real inheritance? The main problem with that approach is that Babel's own _inherits helper uses Object.create! Babel helpers in general are not IE8-compatible, so "just use Babel" does not apply directly to the current problem, which is making React work in IE8 without relying on shams in the environment. (The right solution to using Babel in IE8 without shams, which is what we do in Meteor, is to override the helpers with IE8-compatible versions.) Also, this would be the first use of the class keyword in the entire codebase, which would pull in _inherits and other Babel helpers for the first time, and while there is nothing wrong with that, it seems like a big move that I would rather trust to a core contributor, and not necessarily something to block this issue on.

At the end of the day, setting a proto is 3 lines of JS, or one line with Object.create, but the latter requires that something called Object.create exist in the global environment. There are logically only a few possible ways to avoid calling global Object.create with a small localized code change -- don't use Object.create at all; test for it and use it if available; factor out the functionality you need into a utility -- and any of them is acceptable to me.

Since SyntheticEvent.augmentClass is already basically a one-off inheritance mechanism, in my opinion it would be reasonable to simply inline the code of createObjectWithProto (which either calls Object.create or does the equivalent work) into that function until such time as SyntheticEvent is moved over to real inheritance. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Functionally speaking, there's probably a small benefit to using Object.create if available, because the created object may look better in the dev inspector/debugger, but otherwise there is very little difference.

Copy link
Contributor

Choose a reason for hiding this comment

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

@sebmarkbage was the one who proposed using an ES6 class here, I'll let him make the final call here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just inline the IE8 path in SyntheticEvent.augmentClass.

No need for a fancy module and no need to check for Object.create if the IE8 path is equivalent anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sebmarkbage Will do

@dgreensp
Copy link
Contributor Author

dgreensp commented Oct 6, 2015

Ok, revised.

jimfb added a commit that referenced this pull request Oct 6, 2015
Remove dependence on ES5 shams per #4189
@jimfb jimfb merged commit 0b73099 into facebook:master Oct 6, 2015
@jimfb
Copy link
Contributor

jimfb commented Oct 6, 2015

Thanks!

@sebmarkbage sebmarkbage added this to the 0.15 milestone Oct 6, 2015
@jimfb
Copy link
Contributor

jimfb commented Oct 6, 2015

As per @sebmarkbage, let's take this in 0.15, since we're trying to cut the 0.14 release now and won't have enough time to let this bake.

@zpao
Copy link
Member

zpao commented Oct 6, 2015

Unfortunately GH is annoying and doesn't reopen PRs. Could you create a new one so that we can merge it after branch cut? Thanks! (and sorry for the confusion)

@jimfb
Copy link
Contributor

jimfb commented Oct 6, 2015

Already done. #5063

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants