Remove React's dependency on es5-sham.js.

[dantman]

Shams are potentially dangerous and add 5kb of code, of which React requires almost nothing from (see #4189).

This commit.

• Implements an internal Object.freeze stub that throws like ES5's object freeze and uses Object.freeze if it is implemented.
• Implements an internal Object.create stub that only supports create(prototype), the only Object.create behavior React requires.
• Implements tests for both of these stubs.
• Fixes React to use these stubs internally.
• Removes the early Error thrown when either of these native or shamed methods are not available.
• Removes reference of es5-sham.js from the docs.

These stubs are implemented with consistency in mind so they will throw in the same conditions during development as they will when run in IE8.

Tests which use Object.create or Object.freeze as part of the test have been left alone.

---

I've dropped the reference of es5-sham.js from the docs in this commit. Would it instead be better for the docs to mention a specific range of React versions that require es5-sham.js and what version this commit will be a part of?

[syranide]

This seems superfluous to me at least. Object.create should be enough, a broken environment is not our problem I would think...

[jimfb]

Just throwing this out there, since I'm not sure where everyone stands on this issue: Do we want to be maintaining Object.create and Object.freeze ourselves (increasing codebase size, introducing the possibility of errors); I assume that was the reason we didn't implement it ourselves initially (as we did for Object.assign). Also, I think we're probably getting close to the point of sunsetting support for any browser that doesn't support this functionality natively, right? So why add it this late in the lifecycle?

Having said that, if the sham is causing problems for people and the fix is easy, might as well include it, right?

I don't have a strong preference either way, just curious what @spicyj @zpao think.

[bgw]

Also, I think we're probably getting close to the point of sunsetting support for any browser that doesn't support this functionality natively, right?

To confirm, Object.create and Object.freeze seem to be pretty much everywhere, except IE8.

[dantman]

@jimfb

Do we want to be maintaining Object.create and Object.freeze ourselves (increasing codebase size, introducing the possibility of errors);

• This adds about 180b to the minified code (From what I understand, you could shrink that more if you wanted me to use invariant instead of a 'proper' TypeError).
• These shams are simpler than es5-sham.js' shams (ie: They only implement what is needed).
• Tests for them are included.
• They give explicit errors when you go outside of the bounds of what they support (instead of silently failing in unexpected ways).

In total reducing the possibility of errors IMHO. And they allow you to remove a 5kb (minified) es5-sham.js from your codebase.

So personally, I think the net of this change actually makes errors less likely and reduces the total size of what is required to use React.

EDIT: Here's an example. Currently a developer can carelessly write Object.create(proto, {foo: {get: function() { return 'bar'; }}); in React code. This will run fine on their machine. But once run in IE8 it will throw a TypeError and React will spectacularly break. But if this create shim is used. The developer will get an error on their machine right away that they cannot use the property definitions (or even create(null) with this shim and can immediately avoid breaking React in a supported browser

[sophiebits]

Freeze here seems reasonable-ish but if we want to avoid the create sham I'd just inline the minimal logic like in #139.

Not my fight though. :) @sebmarkbage or @zpao can care.

[jimfb]

+1 for @spicyj's solution. Could lnline both create and freeze if we want to avoid the sham; it sounds like there aren't many of them. Seems like a good way to minimize the surface we're maintaining, which was the intent of my prior comment/question.

Also, as mentioned before:

I don't have a strong preference either way

[sebmarkbage]

Hey I wanted to make Object.assign a required sham. :)

If every library does this then we end up shipping an accumulation of unnecessary polyfills. Very few users of React even need these polyfills because they're not supporting old environments.

Even if they do, they use polyfills anyway for other purposes.

Sure, some libraries respond poorly to polyfills but that doesn't make polyfills inherently bad.

If we go down this route we should use the transpiler model. That way it becomes an optional build process problem. Out of sight - out of mind. That can also be applied to the code base as a whole and integrated into build systems that target different browsers with different packages.

Btw, it is likely that within a year or so, React might stop actively supporting some of these environments like IE8. Not sure but if we have significant need for newer features it might be necessary. That's not on the plans yet but it makes these changes less valuable. The question around ES6 features like Object.assign still stands though.

[dantman]

Sure, some libraries respond poorly to polyfills but that doesn't make polyfills inherently bad.

There is a difference between shims and shams as pollyfills, you're mixing two things together.

A shim implemented in JS like Array.prototype.map will behave so much like a native implementation you can't tell the difference and will work pretty much everywhere, unless you have some pretty broken code.

A sham will sort of behave like the native implementation in the simplest of use cases, and then either silently fail causing potential unexpected bugs or throwing errors when you do something beyond what you can implement without native code. Because a sham introduces an unexpected half-state where an ES5 method is present but isn't actually implemented the way it's supposed to be. Libraries which try and write good code, testing for the presence of ES5 methods before trying to use them, then have a possibility of being broken by the sham.

es5-shim even explicitly says "In many cases, this means that these shams cause many ES5 methods to silently fail. Decide carefully whether this is what you want.". And the problem here is that React is basically taking away the option of deciding whether you want it or not.

If every library does this then we end up shipping an accumulation of unnecessary polyfills.

Then would you prefer that these shams were individual npm modules other libraries could share, without modifying global scope in a way that can break other libraries?

Alternatively, in place of Object.create we could use lodash's .create which implements the behaviour that React requires.

lodash has per-method modules. So we could depend on the lodash.create module (instead of pulling in the entire lodash code). But try to conditionally load require('lodash').create if available, so that if the user already has lodash in their codebase we don't duplicate it.

((I do see the potential problem with browserify's static detection of require() and have a feeling the sollution would probably be a browserify transform that special cases loading of lodash per-method modules))

If we go down this route we should use the transpiler model.

I do like the idea of detecting use of things like Object.create and adapting it. That's also another nice way of detecting potential mistakes like using parts of shams that can't be pollyfilled.

[sebmarkbage]

If you're supporting IE8, you can use a sham that overrides the native Object.create and warns or throws if you use unsupported arguments. That prevents developers from relying on behavior that doesn't work cross-browser. This is the job of the transpilers and polyfills to ensure that you have a uniform environment to work from.

Currently a developer can carelessly write Object.create(proto, {foo: {get: function() { return 'bar'; }}); in React code. This will run fine on their machine.

This will run fine on their machine even with your proposed solution, since they will have access to the native Object.create. Fixing it in React Core doesn't solve the problem for consumers of React. I would argue that if you want to protect against such mistakes, you need to look towards a better polyfilling environment that can provide a consistent debugging experience. (E.g. by forbidding non-polyfillable APIs in all environments.)

Alternatively, in place of Object.create we could use lodash's .create which implements the behaviour that React requires.

This is exactly the problem. If we never adopt better polyfilling strategies, we'll end up having to fallback to community library code like lodash. If we can't use Object.create today, six years after standardization, then we will never be able to have nice things.

I explained why that's a problem in my JSConf.eu talk about this subject:

http://2014.jsconf.eu/speakers/sebastian-markbage-minimal-api-surface-area-learning-patterns-instead-of-frameworks.html

The transpiler strategy is great because it can be used by you as a consumer instead of as part of React's build-process. It maximizes the configurability of optional localized polyfills.

[dantman]

So, browserify transform that modifies Object.create and Object.freeze invocations? Or is babel the proper place... tricky to say for this instance.

In react's repo or as a new npm module?

What do we do when Object.create(proto, {...}) is used (ie: property definitions that can't be simply be pollyfilled)? Silently skip? Error out? Make the transform user pick?

[jimfb]

Wow, I never thought this would spark such a debate :/ I think everyone has made some great points.

If we can't use Object.create today, six years after standardization, then we will never be able to have nice things.

#GreatestQuotes. I really need to start a collection of @sebmarkbage's best quotes :). I agree, btw, fwiw.

So, browserify transform that modifies Object.create and Object.freeze invocations? Or is babel the proper place... tricky to say for this instance.

Probably Babel. It is my understanding that Browserify is more about interaction with NPM whereas Babel is more about language compatibility. This seems like a language compatibility feature.

In react's repo or as a new npm module?

New npm module.

What do we do when Object.create(proto, {...}) is used (ie: property definitions that can't be simply be pollyfilled)? Silently skip? Error out? Make the transform user pick?

Probably warn, but ultimately up to the implementor of the transform.

But we digress. I think the question at hand is: What, if anything, do we want to do about removing the dependency on es5-sham? It sounds like @sebmarkbage is in favor of not re-implementing the polyfill, I find myself leaning in the same direction, so I'm going to mark this PR as needing revision. Implementing this as a transform seems ideal to me, but I'd be perfectly happy if we just manually inlined all the calls to freeze/create.

[sophiebits]

I thought @sebmarkbage says you should use a transform in your app code, that it shouldn't be in the React repo at all.

[sebmarkbage]

Ok so I'm mostly fighting a battle of principals here. However, I have a pragmatic side as well. If we can get a way from using the shams without compromising these principals too much that's a win-win.

If we apply the same logic to ES6 classes, we should be writing using them and then have the transpiler runtime deal with it for us. So, we should just use ES6 classes instead of Object.create, no?

As for Object.freeze, that's a DEV only feature that we use to enforce a set of warnings in one place, and that we're expecting to move/copy into the Babel runtime anyway. I wouldn't be opposed to just conditionally check it in that one callsite. if (Object.freeze) { ... }

[jimfb]

Haha, I think we've reached a consensus here. :)

[sebmarkbage]

We've been talking about best practices for transpiled code and npm. I think we're starting to land at that npm packages should include both the lowest common denominator and the original source. That way it can "just work" for all tools, and then you can get an enhanced experience when tools are aware of additional information. For example, we could leave Object.assign and ES6 classes as original source for build-systems that can handle them, and transpile them in the lowest common denominator bundle.

I guess that means that our current bundle is considered lowest common denominator, and Object.assign should be transpiled/auto-polyfilled.

[dantman]

Thinking about this there are a few messy details about transpiling:

• A transform can't understand that in Object.create(foo) foo might be null. (so it can't just expand to the function Type() {}; Type.prototype = foo; new Type(); form)
• If you opted to make presence of Object.create swap that for inclusion of a shim. You'd need the whole thing, including the larger code attempting to support Object.create(null).
• Even if you split es5-shim/sham into multiple modules (Provide shims as individual files es-shims/es5-shim#175). Object.create kind of depends on Object.defineProperties which depends on Object.defineProperty. (So automatically depending on Object.create without some sort of out-of-band filtering basically pulls in most of es5-sham.js).

Babel does have optional transforms and a loose mode. But in general I see it as still trying to avoid hacks and edge case bugs. And it also seems to focus on leaving non-syntax transforms to polyfills.

So I think this kind of transform falls within browserify scope.

Also, while unfortunate. Because of the quirks of sham dependencies I don't think this will work well as a grand "Use this transform and have all the shims/shams you need automatically transpiled/imported". But instead will be a narrow transform you can include for a simple common use case.

ie:

• You're fine with not having Object.create(null)
• You can guarantee that Object.create(*) will never be null unless Object.create(null) is explicitly used.
• You're fine with Object.create(proto, properties) not being transformed and optionally warning.

And since transforming Object.freeze is really silencing it rather than implementing it via a transform. I'll probably make that a separate browserify-fauxfreeze module.

---

@sebmarkbage Object.assign shims?

• es6-shim
• object.assign

Though I'm not entirely happy about the bloat using either would imply.

[sebmarkbage]

@dantman For Object.freeze I'm fine with just having it be conditional if (Object.freeze) { Object.freeze(this); Object.freeze(this.props); } in the code base. No need to transpile it. Although for other projects it might be useful.

Likewise, we're already using Babel to transpile our source so we should just use the class syntax since that is what we're trying to do there. We don't need a Object.create polyfill. If the resulting code ends up bloated, we can fix it in Babel and everyone benefits.

For example, Babel can track rudimentary type information so if it can easily prove that a value is non-null, it can avoid the check in the transpiled code. @sebmck is very responsive to that kind of challenges.

[sebmck]

@dantman

Babel does have optional transforms and a loose mode. But in general I see it as still trying to avoid hacks and edge case bugs.

Not really. It's a stretch to call any of the semantics you opt out of with loose mode as "edge cases" as sometimes they're cruicail parts of the specific feature.

Because of the quirks of sham dependencies I don't think this will work well as a grand "Use this transform and have all the shims/shams you need automatically transpiled/imported". But instead will be a narrow transform you can include for a simple common use case.

You would be extremely strict and err on the side of caution with your transformations and then optimise for the common case where you can be 100% confident. At least this is the approach I take when optimising transforms and what I believe @sebmarkbage was alluding to.

@sebmarkbage

For example, Babel can track rudimentary type information so if it can easily prove that a value is non-null, it can avoid the check in the transpiled code.

Sort of. There currently aren't APIs for performing type checks like that. Right now there are only binary "is this a specific base type" methods. They'll definently need to be introduced in the future though, either when Babel gets better type inference (unlikely) or when there's tighter integration with Flow (hopefully!).

[syranide]

If you're supporting IE8, you can use a sham that overrides the native Object.create and warns or throws if you use unsupported arguments. That prevents developers from relying on behavior that doesn't work cross-browser. This is the job of the transpilers and polyfills to ensure that you have a uniform environment to work from.

I know the discussion is already past this, but isn't that exactly the problem? To illustrate, by that argument it would kind of make sense to implement Object.freeze and have it throw because it's not supported. But no library in existence feature detects Object.freeze by try/catch because that behavior is not defined by the standard, they test if (Object.freeze) {, crash.

So it's more that shams is a flawed concept, you deviate from the standard and polyfill the environment with non-standard/partial implementations. For you as a developer it works out really rather well, but as a consumer of various libraries that rely on standard behavior it's a nightmare.

[jimfb]

@syranide If you're calling into a sham that can't be polyfilled, you can wrap the invocation in a try-catch, so the exception will be non-fatal. You are effectively handling the case where the operation is not supported by that browser. It's a pretty reasonable case for developers to handle, since they know they are invoking features that are not well supported by their target browsers.

[syranide]

If you're calling into a sham that can't be polyfilled, you can wrap the invocation in a try-catch, so the exception will be non-fatal. You are effectively handling the case where the operation is not supported by that browser. It's a pretty reasonable case for developers to handle, since they know they are invoking features that are not well supported by their target browsers.

@jimfb Not arguing that it can't be tested for by try/catch, the reality is that it often isn't (for whatever reason).

[jimfb]

The only way that will change is if people use shims more often :).

But yeah, I think we're all in agreement that shims/shams suck and the best solution is to use a compiler/transpiler that generates code for your target browsers.

[syranide]

@jimfb I don't mind shims. As far as I've been able to understand it "transpiling shims" doesn't really work super well. It works great for your own code, but you end up polyfilling the environment too because there are other libraries out there aren't transpiled along with the rest of your code.

So to me shims seem just fine, but not all shams, people have different expectations when it comes to shams and that's where it breaks down.

[sebmarkbage]

If your library relies on defineProperty behavior then it won't work in IE8 anyway.

The issue is that libraries might have the ability to work around the sham if it is not available and therefore they want to feature detect if it does work, then conditionally fallback.

That notion is flawed because now your code will still behave differently, just using one indirection. So effectively to get a safe environment you need to ensure that all your libraries conform to a subset of the language. In which case the sham model works fine.

[syranide]

That notion is flawed because now your code will still behave differently, just using one indirection.

No, it will explicitly do what's written in the code, it will not perform some arbitrary fallback behavior that I may or may find acceptable and have to guard against too. To take a silly example, why is it that Object.freeze should silently return? Why can't it throw an error if XYZ? Why does React not feature test that and return silently if the provided behavior unacceptable?

So effectively to get a safe environment you need to ensure that all your libraries conform to a subset of the language. In which case the sham model works fine.

"ensure that all your libraries conform", why it's so obviously fine for React to make non-standard demands on my environment? Why can't React conform to the standard instead?

To me it's really simple; follow the standard then everyone gets the same experience and everyone gets to choose their preferred fallback, it's simple and it works. Don't follow the standard and you're making vague demands on everyone else to elaborately feature test for what is a minor benefit. It would have been fantastic if ECMA provided specs for acceptable partial implementations, but they didn't.

[sebmarkbage]

I guess that we actually agree. Shams should throw if they can't follow the implemented behavior rather than silently failing. E.g. if the sham can't implement the second argument of Object.create, it should throw. Libraries that doesn't depend on that behavior should just avoid passing the second argument.

I agree that silently failing is generally a bad strategy for a sham, however, for perf reasons it might be prohibitively expensive to throw. In those cases, you have a tough choice. You could take the extreme route and say that we should never use those APIs at all and implement it in user space, but you could be talking about an edge case that almost never occurs. We hold up progress in adoption because of it.

My preference is to use a DEV time warning to ensure that this behavior isn't being relied on, but that does not allow the try/catch pattern to be used in production code.

One example is ES6 classes. They require methods to be non-enumerable. Making them non-enumerable is prohibitively slow. Should we not use ES6 classes because of it? No, we definitely should be using them anyway with the loose mode because iterating over them is an edge case. It is up to the polyfilling environment to ensure that you have a good DEV mode experience that warns you if you try to iterate over the properties of a class prototype.

It is not as simple as just follow the standard or don't use it at all.

[sebmarkbage]

As for Object.freeze, this doesn't have to throw. You can just avoid adding it. That way it can be ignored. However, I think that this particular case is fine to polyfill as a noop.

You shouldn't be relying on try/catch for code flow. In fact, the spec relies on throwing in places that are expected to NOT throw in the future. So you will break future specs by doing that.

You also shouldn't be using non-strict mode. That's just breaks everything.

So given those assumptions, your Object.freeze dependent code will behave the same in your modern environment as a sham:ed environment. This is a special case that mostly only applies to freeze but every rule has its exceptions.

Besides, try/catch as a feature detection is a terrible user experience. Map polyfills rely on it to test the constructor, but we've had to disable those polyfills since they cause the "Break on caught exceptions" mode to trigger in Chrome. A few polyfills and that important debug tool is busted.

[jimfb]

@dantman @sebmarkbage To summarize: it sounds like the ideal solution is a transform but we would happily accept inlining the ifexists check for freeze and es6 classes instead of create, right?

[dgreensp]

+1 for not requiring an environment where Object.freeze exists, even if it is non-functional! That can't be right.

[sebmarkbage]

Closing out in favor of #4189