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

Make React run without es5-sham #4189

Closed
dantman opened this issue Jun 22, 2015 · 6 comments
Closed

Make React run without es5-sham #4189

dantman opened this issue Jun 22, 2015 · 6 comments

Comments

@dantman
Copy link
Contributor

dantman commented Jun 22, 2015

I'm ok with React requiring shims. I'd include them whether React needed them or not. However I don't believe React should require es5-sham to work. Because, among other reasons, including shams has the potential to break other libraries on the same page and hence doesn't sound like a good practice for one library to demand.

Of the two shams React says it requires (in the 0.14 code in npm) each one is only used once in the entire codebase.

2 function invocations doesn't sound like a good reason for requiring a 5kb library that has potential side effects.

The best practice for React would probably be to provide its own small module providing either the native function or an internal sham.

This would also be a positive. As these internal shams wouldn't have to waste bytes trying to sham functionality that React doesn't need. And they could emit invariant warnings when misused during development instead of failing (potentially silently) the way they would in es5-sham.

@dantman
Copy link
Contributor Author

dantman commented Jun 22, 2015

I'm trying out implementing this myself. And I'm discovering some interesting quirks and possibilities.


Firstly, in ES5 Object.freeze throws if given a non-object, in ES6 it returns it.

Which means on one dev's machine in a browser implementing ES6 Object.freeze could run fine over quirky code that accidentally calls Object.freeze on a primitive. And then fail spectacularly in production in a browser that implements ES5.

Which makes the React freeze stub a good place to even out the behaviour. Because calling freeze on a non-object in React code is most likely a mistake I've opted to throw the TypeError.


Secondly, every place in code that uses Object.freeze does it in development only.

Would it be good or unexpected to make React's internal freeze only run in dev?


Last is a question. Do we want freeze to display a warning during development that freeze is not implemented?

dantman added a commit to dantman/react that referenced this issue Jun 22, 2015
Shams are potentially dangerous and add 5kb of code, of which React requires almost nothing from (see facebook#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.
@syranide
Copy link
Contributor

👍 I'd love this too, es5-sham doesn't interop very neatly with all libraries so I've ended up in weird situations where I (luckily) could include certain libraries before es5-sham and others after and have it sorted out that way. But I would really like not to have to use es5-sham at all.

@desenmeng
Copy link

👍

1 similar comment
@Passerby
Copy link

👍

@jimfb
Copy link
Contributor

jimfb commented Jul 10, 2015

To summarize the discussions in #4192 : 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. See #4192 for relevant discussions.

dgreensp added a commit to meteor/react-packages that referenced this issue Jul 21, 2015
dgreensp added a commit to dgreensp/react that referenced this issue Sep 24, 2015
@dgreensp
Copy link
Contributor

I submitted a fresh PR for review: #4959

The changes are very small and straightforward.

dgreensp added a commit to dgreensp/react that referenced this issue Oct 6, 2015
jimfb added a commit that referenced this issue Oct 6, 2015
Remove dependence on ES5 shams per #4189
jimfb added a commit that referenced this issue Oct 6, 2015
Revert "Remove dependence on ES5 shams per #4189"
jimfb added a commit that referenced this issue Oct 7, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants