Fix .includes issue once and for all

[bestander]

Do you want to request a feature or report a bug?
Node 4 compat issue

What is the current behavior?

1. Jest tests on Node 4 don't catch compatibility issues like having .includes in -legacy.js builds.
2. We need to include proper polyfil for Node 4

[Volune]

The simple way would be to add a dependency to babel-polyfill and require it at the beginning.

Node 4 already supports several ES6 features (compatibility table). I think it would be better to add a dependency to core-js, and only require the necessary polyfills.

core-js is internally used by babel-polyfill and babel-runtime. babel-runtime is already used in Yarn, but does not handle instance functions such as Array.prototype.includes.

[bestander]

@Volune, would you send a PR for that please?

[kaylie-alexa]

I also think that babel-polyfill is quite heavy, if you want to fix just .includes issue, then we should use airbnb's .includes polyfill https://www.npmjs.com/package/array-includes.

[bestander]

Makes sense. On top of that we need to make jest run without polyfills, these issues should be caught by CI

…

On 22 June 2017 at 22:19, kaylieEB ***@***.***> wrote: I also think that babel-polyfill is quite heavy, if you want to fix just .includes issue, then we should use airbnb's .includes polyfill https://www.npmjs.com/package/array-includes. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#3693 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACBdWFXEfnqgT1vr_YKiOSY84G9tUbl_ks5sG0rcgaJpZM4OBv9s> .

[kaylie-alexa]

hm, looking at babelrc
it looks like jest already runs without the polyfill? I actually think we just need to add a plugin
to the pre-node setting. Happy to send a PR.

[bestander]

Go ahead, thanks

…

On Thu, Jun 22, 2017 at 10:41 PM kaylieEB ***@***.***> wrote: hm, looking at babelrc <https://github.com/yarnpkg/yarn/blob/c844df7236ec122ca6b803b9f2943352c9b09a7e/.babelrc> it looks like jest already runs without the polyfill? I actually think we just need to add a plugin <https://www.npmjs.com/package/babel-plugin-array-includes> to the pre-node setting. Happy to send a PR. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#3693 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACBdWGk_dYQ_ucADdelxyOmytAogJQOuks5sG1AcgaJpZM4OBv9s> .

[kaylie-alexa]

Thanks! #3705

[Volune]

I think we should have a look at babel-preset-env to fix all similar potential issues at once.

[voxsim]

@bestander here, #3652, I upgraded babel and used babel-preset-env.. If you want I can move some commits and arrange a pull request for the next week. This is propedeutic for the new release of Jest too.
I saw the pull request #3705 and imho we should avoid include babel plugins that are not followed and upgraded as well, otherwise next upgrades can be a nightmare O.o
If we want to transpile for node 4 we should do what @Volune says, btw there will be always some pitfalls for what I understand (I read the readme of https://www.npmjs.com/package/babel-plugin-array-includes and the discussion here avajs/ava#263, very interesting) and I think we should avoid Array.prototype.includes in both src and tests files and just write a function includes in our code :P

[bestander]

Thanks for chiming in @voxsim.

A few points.

1. I think it is bad to allow Node 4 to hold us back.

2. Jest in Node 4 mode should run with the same babel settings as production builds of Yarn.
   Otherwise there is less value in testing in Node 4.
   However I understand there may be some configuration cost to this.

3. Could you explain what is the problem with babel-preset-env?

[kaylie-alexa]

@voxsim I agree we should move to babel-preset-env regardless of support for .includes, to be futureproof. Per the original issue, the only way to use .includes is to include babel-polyfill or a syntax plugin like the one I added, and preset-env doesn't provide it out of the box. But the use of BuiltIns looks promising https://babeljs.io/docs/plugins/preset-env/#optionsuse-built-ins. It will conditionally require the needed polyfills base on the environment using babel-polyfill, so maybe we can get both?

[voxsim]

@kaylieEB @bestander I opened a pull request with an experiment that seems to work O.o I am using babel-polyfill and babel-preset-env, please let me know if you like it :)

[bestander]

❤️

[BYK]

@bestander - I think we can close this issue now? Since Node 4 is an LTS and will be maintained until April 2018, I'd say we should support it. At least until 1.0 is out. I think the polyfill is a bit heavy handed and now CI should catch the issues so no further action needed?

[bestander]

Up to you, @BYK.
The issue is at least mitigated.
Personally I'd like to be able to use .includes in the code one way or the other.