Add ES6 Symbol polyfill for compatibility with iOS 8 and Android #5294

skevy · 2016-01-13T21:40:09Z

facebook-github-bot · 2016-01-13T21:40:37Z

By analyzing the blame information on this pull request, we identified @cpojer, @martinbigio and @sahrens to be potential reviewers.

martinbigio · 2016-01-14T03:17:24Z

cc @davidaurelio

mkonicek · 2016-01-14T16:05:27Z

Thanks a lot for the fix @skevy!

@davidaurelio How do we merge PRs that add dependencies now that we have the new infra for storing node_modules internally?

For context, we have a custom way to fetch node_modules at fb because React Native is used in the main fb apps and there are engineers working on those apps who don't deal with JS, npm, etc. (we also don't want CI servers to run 'npm install' because it takes forever :)). We used to check node_modules into the Mercurial repo but maintaining that proved to be a pain so we have a new system to do that.

davidaurelio · 2016-01-14T16:11:13Z

@mkonicek I guess for now we have to have these imported by a FB employee and do the necessary things. We could enhance the bot to a) require that or b) do the necessary things itself.

davidaurelio · 2016-01-14T16:12:06Z

Libraries/JavaScriptAppEngine/Initialization/InitializeJavaScriptAppEngine.js

@@ -126,6 +126,10 @@ function setUpAlert() {
  }
 }

+function setUpSymbol() {
+  GLOBAL.Symbol = require('es6-symbol');


I don’t think we want to overwrite native Symbol if present.

i.e. use require('es6-symbol/implement');

https://github.com/medikoo/es6-symbol/blob/master/index.js#L3

Am I reading that wrong? It seems like it's checking for native, global Symbol before it polyfills.

Or would you rather have that check in our code?

I’m sorry, I misunderstood their documentation.

@davidaurelio np :) If you'd rather me move the check into RN so it's more clear, I'm happy to do that!

no, it’s good the way it is. @mkonicek will take care of merging

Thanks gentlemen!

mkonicek · 2016-01-15T11:53:35Z

@facebook-github-bot import

facebook-github-bot · 2016-01-15T11:54:24Z

Thanks for importing. If you are an FB employee go to https://our.intern.facebook.com/intern/opensource/github/pull_request/1107166042641372/int_phab to review.

mkonicek · 2016-01-15T19:18:11Z

I just did npm install --save [email protected] which added 972 files to node_modules, making React Native JS deps more bloated and making the JS bundle bigger. Maybe this is how the JS ecosystem works but adding so much code to polyfill Symbol worth it? :(

Here are all the files: http://pastebin.com/wyePxHtd

cc @vjeux, @davidaurelio

ide · 2016-01-15T19:26:56Z

Could we use core-js instead of es6-symbol, since that's what Babel uses (or used to use...) and should get deduped with Babel?

satya164 · 2016-01-17T12:34:34Z

What about using babel-polyfill (which uses core-js)?

skevy · 2016-01-17T15:39:16Z

@mkonicek sorry I didn't see this. I'll take a look.

I'm sure we can figure out a different solution.

We actually do already have Babel-polyfill in deps (not actually sure why)...I was just hesitant of using that as it adds a lot of code to people's bundles and doesn't really add that much (since we polyfill a bunch anyway).

I'm AFK at the moment, but my intuition tells me that either we can vendor a symbol polyfill or try core-JS...

skevy · 2016-01-17T15:40:00Z

Honestly, I wouldn't be surprised if the deps that es6-symbol added are deps we already have somewhere in the tree...but that's not how npm works. :-(

mkonicek · 2016-01-18T13:45:34Z

@skevy Yes I was just worried when I saw this was adding files like node_modules/es6-symbol/node_modules/es5-ext/math/asinh/implement.js that are very likely not needed.

I have little context, just read the Symbol docs last week. If you guys more familiar with JS are happy with merging this go ahead. I was just asking whether adding all this code was worth it, and you're right all of it might already be in the tree :(

It's frustrating to see the node_modules folder is 310MB (!) for a minimal RN app (with npm2), and I understand that npm3 is even slower than npm2 so we can't recommend it yet.

skevy · 2016-01-18T16:46:33Z

@mkonicek totally agreed. Hold off on merging this everyone - I'm gonna just vendorize this polyfill -- absolutely no reason to add a huge dep.

skevy · 2016-01-18T16:47:12Z

@mkonicek I am going to do this right now though, so hopefully we can still get this into RN 0.18...would be a good fix as this causes a problem on < iOS 9 and Android (some or all...not sure).

skevy · 2016-01-18T17:15:08Z

OK so actually, two comments on this:

Like @ide says, we could use core-js here. I don't think including core-js as a dependency is a good solution though, as that would still contribute to node-module bloat (at least on npm2...npm3 would de-dupe if we use the same semver range as babel). But, we could do a custom build of core-js with the Symbol polyfill only. I just did that...it's 24KB. Problem is, the code is pretty unreadable (it's webpack generated). I usually wouldn't mind this, since it's just a polyfill and no one is liable to touch it...but all the other polyfills that currently are vendorized in RN are extremely readable.
We're already currently including babel-polyfill as a dependency. I've used babel-polyfill in my projects, and it's caused no issues. It would add some bloat to the actual bundle size...I can see how much if you guys think it would be reasonable to explore this option. @satya164

ide · 2016-01-18T17:50:58Z

The size of the bundle is more important than the size of node_modules here. npm uses gzip and npm 3 is not bandwidth-constrained, and 3xxMB is a fraction of a percent of modern HDD space. So whatever polyfill gives us some combination of compact and clean code is actually my preference, whether that's core-js or something else.

skevy · 2016-01-18T17:53:47Z

I'm talking to @vjeux about this...will report back.

vjeux · 2016-01-18T18:54:49Z

Here's the plan:

Import the babel-plugin-symbol-member transform that we have internally: https://gist.github.com/vjeux/2715c9687653547037b0

It just replaces Symbol.iterator with typeof Symbol.iterator === 'function' ? Symbol.iterator : '@@iterator' which makes for..of work. No need to include a 24kb polyfill since the only use case is to have this variable set.

Update the JavaScript environment docs to say that we support for..of and mention the existence of this "polyfill". http://facebook.github.io/react-native/docs/javascript-environment.html#content

mkonicek · 2016-01-18T22:01:43Z

Nice! I've released 0.18.0. Can anyone release 0.18.1 with the single extra commit? Since it's a JS-only change, it's just npm publish.

satya164 · 2016-01-22T21:41:52Z

ping @bestander :)

skevy · 2016-01-25T15:53:59Z

@DmitrySoshnikov what would be the specific point of defining Symbol.iterator on Array.prototype, if we're using the loose mode version of for..of? The loose mode transform covers both the case where Symbol does exist on the platform, and when it does not. Is there another use case for Symbol.iterator on Array besides for..of?

DmitrySoshnikov · 2016-01-26T07:42:28Z

@skevy, I meant in case if you don't wanna use loose mode. Otherwise it should be fine (we don't have Symbol.iterator on Array.prototype internally).

skevy · 2016-01-26T15:08:01Z

@DmitrySoshnikov great. We'll go with using loose mode + documenting what we do and don't support.

@vjeux you good with this plan?

vjeux · 2016-01-26T15:13:02Z

Works for me, Dmitry is the pro here :p

skevy · 2016-01-26T16:18:30Z

Ok - I'm going to close this then in favor of adding loose mode in #5214 and then I'll open a new PR with doc changes.

Summary: Turns out, even after discussion that was had in #5294 (comment), we really do need this transform. I've just included it in the preset...let me know if you all would rather publish to npm. The actual reason why this is necessary is because in the latest sync from FB, fbjs was updated to use the `Symbol.iterator` express in it's isEmpty function: facebook/fbjs@064a484 We use this in RN in the ListView...and this change (once #5084 is merged) will cause ListView to break on older JSC context's. This resolves that, and is probably something we should have had all along. Closes #5824 Reviewed By: svcscm Differential Revision: D2913315 Pulled By: vjeux fb-gh-sync-id: abaf484a9431b3111e8118d01db8d2c0d2dd73ca shipit-source-id: abaf484a9431b3111e8118d01db8d2c0d2dd73ca

peterlazar1993 · 2016-02-22T06:20:34Z

I'm still facing this issue, using redux-saga on Android on react-native v0.20. This works in debug mode though.

ms88privat · 2016-02-28T01:21:00Z

👍

lrettig · 2016-02-29T06:23:38Z

I upgraded to RN 0.20 and I'm now getting a different error. My code looks like this:

for (let el of myArray) { ... }

and it appears to transpile into this:

for (var _iterator=myArray[typeof Symbol==='function'?Symbol.iterator:'@@iterator'](),_step; !(_iteratorNormalCompletion=(_step=_iterator.next()).done); _iteratorNormalCompletion=true) { var el=_step.value; ... }

which produces this error:

TypeError: undefined is not a function (evaluating 'myArray[typeof Symbol==='function'?Symbol.iterator:'@@iterator']()')

Indeed, my array doesn't contain functions, so I don't understand how this transpiled code is supposed to work. Does anyone know what's going on?

Thanks.

skevy · 2016-02-29T06:30:39Z

@lrettig in JS environments that support Symbol, the prototype of your Array would indeed contain a function: myArray[Symbol.iterator]. But in non JS envs, that Symbol.iterator is simply added to the Array prototype as @@iterator...so that's what this is checking.

I'm not sure why that's not working for you...are you using any custom babelrc or any type of custom config?

lrettig · 2016-02-29T06:33:48Z

There was indeed some junk in my .babelrc that must have caused this. I removed it, and it started working. Thanks!

skevy · 2016-02-29T06:34:41Z

So the reason, for what it's worth, that it wasn't working...is because your code was not being transpiled into the "for...of" loose mode code.

This is important for non-Symbol supporting JS platforms.

lrettig · 2016-02-29T06:37:25Z

I suspect the culprit was this entry in my .babelrc "plugins":
"transform-es2015-for-of" (probably from an earlier attempt to fix the same
problem).

On Mon, Feb 29, 2016 at 1:36 AM, Adam Miskiewicz [email protected]
wrote:

So the reason, for what it's worth, that it wasn't working...is because
your code was not being transpiled into the "for...of" loose mode code.

This is important for non-Symbol supporting JS platforms.

—
Reply to this email directly or view it on GitHub
#5294 (comment)
.

skevy · 2016-02-29T06:42:27Z

Note that we have a preset now...called "babel-preset-react-native" that you can install and use in your babelrc, rather than copying the RN config or using the extends babelrc option.

This ensures that you're using all the proper transforms that RN requires.

davide-scalzo · 2016-03-06T14:13:04Z

related to this by any chance? gaearon/react-proxy#50

Summary: Turns out, even after discussion that was had in facebook#5294 (comment), we really do need this transform. I've just included it in the preset...let me know if you all would rather publish to npm. The actual reason why this is necessary is because in the latest sync from FB, fbjs was updated to use the `Symbol.iterator` express in it's isEmpty function: facebook/fbjs@064a484 We use this in RN in the ListView...and this change (once facebook#5084 is merged) will cause ListView to break on older JSC context's. This resolves that, and is probably something we should have had all along. Closes facebook#5824 Reviewed By: svcscm Differential Revision: D2913315 Pulled By: vjeux fb-gh-sync-id: abaf484a9431b3111e8118d01db8d2c0d2dd73ca shipit-source-id: abaf484a9431b3111e8118d01db8d2c0d2dd73ca

sdeleon28 · 2018-03-16T22:58:17Z

This missing polyfill causes this weird behavior with immutable immutable-js/immutable-js#1474

Thought I'd leave it here for reference

Add ES6 Symbol polyfill for compatibility with iOS 8 and Android

36f56c0

facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Jan 13, 2016

skevy mentioned this pull request Jan 13, 2016

[0.18] Things we should cherry-pick to 0.18 #5201

Closed

11 tasks

davidaurelio reviewed Jan 14, 2016
View reviewed changes

davidaurelio added GH Review: needs-revision and removed GH Review: review-needed labels Jan 14, 2016

satya164 added the Import Failed label Jan 22, 2016

skevy added a commit to skevy/react-native that referenced this pull request Jan 25, 2016

Making for..of transform loose, per discussions in facebook#5294

5b8d7c8

skevy mentioned this pull request Jan 25, 2016

Use "babel-preset-react-native" #5214

Closed

skevy closed this Jan 26, 2016

skevy added a commit to skevy/react-native that referenced this pull request Jan 28, 2016

Making for..of transform loose, per discussions in facebook#5294

fa52303

skevy added a commit to skevy/react-native that referenced this pull request Jan 28, 2016

Making for..of transform loose, per discussions in facebook#5294

078a723

skevy added a commit to skevy/react-native that referenced this pull request Jan 29, 2016

Making for..of transform loose, per discussions in facebook#5294

7e253d5

skevy added a commit to skevy/react-native that referenced this pull request Jan 29, 2016

Making for..of transform loose, per discussions in facebook#5294

099f643

skevy added a commit to skevy/react-native that referenced this pull request Jan 30, 2016

Making for..of transform loose, per discussions in facebook#5294

8f715d7

oviava mentioned this pull request Feb 3, 2016

Can't find variable symbol lenaten/react-native-relay#9

Open

skevy mentioned this pull request Feb 8, 2016

Adding 'transform-symbol-member' transform to preset. #5824

Closed

davide-scalzo mentioned this pull request Mar 6, 2016

core-js WeakMaps are broken on React Native? gaearon/react-proxy#50

Closed

chrusart mentioned this pull request Aug 3, 2018

Does it work with react native? ttag-org/ttag#114

Closed

Add ES6 Symbol polyfill for compatibility with iOS 8 and Android #5294

Add ES6 Symbol polyfill for compatibility with iOS 8 and Android #5294

Conversation

skevy commented Jan 13, 2016

facebook-github-bot commented Jan 13, 2016

martinbigio commented Jan 14, 2016

mkonicek commented Jan 14, 2016

davidaurelio commented Jan 14, 2016

davidaurelio Jan 14, 2016

Choose a reason for hiding this comment

davidaurelio Jan 14, 2016

Choose a reason for hiding this comment

skevy Jan 14, 2016

Choose a reason for hiding this comment

davidaurelio Jan 14, 2016

Choose a reason for hiding this comment

skevy Jan 14, 2016

Choose a reason for hiding this comment

davidaurelio Jan 14, 2016

Choose a reason for hiding this comment

skevy Jan 14, 2016

Choose a reason for hiding this comment

mkonicek commented Jan 15, 2016

facebook-github-bot commented Jan 15, 2016

mkonicek commented Jan 15, 2016

ide commented Jan 15, 2016

satya164 commented Jan 17, 2016

skevy commented Jan 17, 2016

skevy commented Jan 17, 2016

mkonicek commented Jan 18, 2016

skevy commented Jan 18, 2016

skevy commented Jan 18, 2016

skevy commented Jan 18, 2016

ide commented Jan 18, 2016

skevy commented Jan 18, 2016

vjeux commented Jan 18, 2016

mkonicek commented Jan 18, 2016

satya164 commented Jan 22, 2016

skevy commented Jan 25, 2016

DmitrySoshnikov commented Jan 26, 2016

skevy commented Jan 26, 2016

vjeux commented Jan 26, 2016

skevy commented Jan 26, 2016

peterlazar1993 commented Feb 22, 2016

ms88privat commented Feb 28, 2016

lrettig commented Feb 29, 2016

skevy commented Feb 29, 2016

lrettig commented Feb 29, 2016

skevy commented Feb 29, 2016

lrettig commented Feb 29, 2016

skevy commented Feb 29, 2016

davide-scalzo commented Mar 6, 2016

sdeleon28 commented Mar 16, 2018