Add ES6 Symbol polyfill for compatibility with iOS 8 and Android

[skevy]

Fixes#4676

[facebook-github-bot]

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

[martinbigio]

cc @davidaurelio

[mkonicek]

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]

@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]

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

[davidaurelio]

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

[skevy]

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?

[davidaurelio]

I’m sorry, I misunderstood their documentation.

[skevy]

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

[davidaurelio]

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

[skevy]

Thanks gentlemen!

[mkonicek]

@facebook-github-bot import

[facebook-github-bot]

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]

I just did npm install --save symbol@3.0.2 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]

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]

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

[skevy]

@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]

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]

@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]

@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]

@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]

OK so actually, two comments on this:

1. 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.

2. 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]

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]

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

[vjeux]

Here's the plan:

1. 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.

2. 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]

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]

ping @bestander :)

[skevy]

@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]

@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]

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

@vjeux you good with this plan?

[vjeux]

Works for me, Dmitry is the pro here :p

[skevy]

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.

[peterlazar1993]

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

[ms88privat]

👍

[lrettig]

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]

@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]

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

[skevy]

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]

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 notifications@github.com
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]

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]

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

[sdeleon28]

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

Thought I'd leave it here for reference