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

Enable babel-preset-env for node_modules that target newer Node versions #1125

Closed
julien-f opened this Issue Dec 1, 2016 · 70 comments

Comments

Projects
@julien-f
Copy link

julien-f commented Dec 1, 2016

Hello,

More and more npm packages contain ES2015+ code (for now usually features supported by Node 4 but i guess it will move to Node 6 features once 4 will be unmaintained). Unfortunately, this breaks compatibility with Uglify and browsers.

The only answer to this is, IMHO, to also transpile external dependencies. Is this supported by create-react-app?

I'm asking because my own build set up is getting confronted to this and I'm thinking about migrating to create-react-app :)

@gaearon

This comment has been minimized.

Copy link
Member

gaearon commented Dec 1, 2016

No, this is not supported because builds will get much slower. We don't recommend using libraries that don't precompile their code to ES5 but are supposed to be usable in ES5.

We will fix the Uglify problem in the future by using Babili. However it still means that even uglified code will still be ES6 if that's what your dependencies are shipping.

With time it won't matter as much because mainstream browsers will support ES6. Until then I recommend to use libraries that take care of transpilation step.

@gaearon gaearon closed this Dec 1, 2016

@julien-f

This comment has been minimized.

Copy link
Author

julien-f commented Dec 1, 2016

I understand but it creates a dichotomy between Node's and browsers' worlds, which will lead to less isomorphic JavaScript components/libraries :/

@sindresorhus, you have migrated many of your packages to ES2015, what's your opinion on this?

And, once Uglify as been replaced by Babili, I'm not sure it'll have a big impact to add other Babel presets to the compilation pass (@hzoo, any inputs on this?)

@julien-f

This comment has been minimized.

Copy link
Author

julien-f commented Dec 1, 2016

Concerning the perf impact, I guess it depends of how many times the code is parsed/generated, it would be interesting to have everything done in a single one but it probably depends on the bundler (Webpack).

@gaearon gaearon reopened this Dec 1, 2016

@gaearon

This comment has been minimized.

Copy link
Member

gaearon commented Dec 1, 2016

OK, let’s keep it open and wait for more feedback.

The cold start time is pretty bad. I saw an increase from 6 to 20 seconds on a relatively small app.

However, since we enable caching for Babel loader, warm start stays 6 second.
So maybe it’s not such a big deal.

@gaearon gaearon changed the title [Question] are external deps compiled down to ES5? Enable Babel for node_modules Dec 1, 2016

@gaearon

This comment has been minimized.

Copy link
Member

gaearon commented Dec 1, 2016

I would be comfortable doing this if:

  • We are sure that enabling Babel never breaks third party code. I’m actually not at all sure this is true.
  • We are sure only cold start suffers, both for npm start and npm run build, and that whatever caches we are using work well for next start, for edits while it is running, and for builds.
@julien-f

This comment has been minimized.

Copy link
Author

julien-f commented Dec 1, 2016

I understand very well your pain about long compile time, but I think the answer is somewhere because with bundling, tree-shaking and minification, this code is already parsed.

Maybe the work is more on the ecosystem side (Babel, Webpack, etc.) to avoid doing unnecessary operations (parsing and generation).

@hzoo

This comment has been minimized.

Copy link

hzoo commented Dec 1, 2016

Running babel on all of node_modules does sound really slow - right it might be faster (would need to measure) passing an AST to webpack or vice versa.

@julien-f

This comment has been minimized.

Copy link
Author

julien-f commented Dec 1, 2016

@hzoo but if Babili is used, the code won't get much slower by adding other Babel plugins/presets right?

At least much faster that Babel + Uglify.

@gaearon

This comment has been minimized.

Copy link
Member

gaearon commented Dec 1, 2016

I understand very well your pain about long compile time, but I think the answer is somewhere because with bundling, tree-shaking and minification, this code is already parsed.

To be fair there is no minification in development.

@julien-f

This comment has been minimized.

Copy link
Author

julien-f commented Dec 1, 2016

To be fair there is no minification in development.

Seems logic, but watch mode may be enough to mitigate the issue…

@gaearon

This comment has been minimized.

Copy link
Member

gaearon commented Dec 1, 2016

At this point I’m more concerned about correctness than speed.
My gut tells me some projects will break if you Babel them.

@julien-f

This comment has been minimized.

Copy link
Author

julien-f commented Dec 1, 2016

Yes, that's the main issue.
Maybe we can just enable a limited number of plugins on external deps, like those in babel-preset-latest.

I don't know if running Babel on external deps is a good idea, but I fear the problem of incompatible deps will continue to grow, and fast. :/

@gaearon

This comment has been minimized.

Copy link
Member

gaearon commented Dec 1, 2016

I think this should be on the library authors because they know which JS version they wrote code in, and they can spend their CPU once every release so that other people don’t have to waste their CPU compiling their libraries. It is not difficult to compile JS code to ES5 as a build step, with Babel it's literally a single command. And they can always provide multiple versions (e.g. my-library/es5 or something).

@julien-f

This comment has been minimized.

Copy link
Author

julien-f commented Dec 1, 2016

Yes, but they might do this because it's easier, or because the code is faster (generators, async functions) if not transpiled, or simply because they did not thought about the use of their code in something else than Node.

They also might say that it's up to the consumers to compile because only them know which JS version they target ;)

@sindresorhus, when you have time, I would love to have your opinion on this :)

@hzoo

This comment has been minimized.

Copy link

hzoo commented Dec 1, 2016

I think it's really weird to be compiling node_modules. Libraries/packages should be compiling themselves or at least providing an optional es5 version?

This is also what caused babel/babylon#154 - where you would have an issue in 3rd party code

@gaearon

This comment has been minimized.

Copy link
Member

gaearon commented Dec 1, 2016

The problem is the shifting baseline. Why are we using ES5 as a baseline? Because it was the lowest common denominator. But it’s shifting, and it won’t make sense to still use ES5 as lowest common denominator in 10 years. So the question is: when should the lowest common denominator shift to ES6?

I don’t think the answer is now, and I also don’t think it’s a good idea to compile ES6 code in dependencies. It’s just asking for random breakage. I think it’s up to dependencies to declare which environments they support, and if they choose to ship ES6 as main then it’s their decision to be incompatible with older browsers.

@julien-f

This comment has been minimized.

Copy link
Author

julien-f commented Dec 1, 2016

Maybe the user could configure a list of packages to compile?
But then there is the question of their own dependencies… 😢

@sindresorhus

This comment has been minimized.

Copy link

sindresorhus commented Dec 1, 2016

I think you're asking too much of module maintainers. I target Node.js 4 for modules I make. If users wants them to run in older browsers, it's up to them to transpile. I'm not interested in having a compile step in all my modules.

Maybe there could be a Babel feature (or another tool) that checks the "engine" package.json field in dependencies and decides which dependencies to transpile. Caching should also help, so a dependency is only transpiled once and cached forever for that version.

I've expanded on my answer here: sindresorhus/ama#446

@wtgtybhertgeghgtwtg

This comment has been minimized.

Copy link
Contributor

wtgtybhertgeghgtwtg commented Dec 1, 2016

Maybe there could be a Babel feature (or another tool) that checks the "engine" package.json field in dependencies and decides which dependencies to transpile.

Wouldn't that be more of a webpack thing? The browser version might need to be transpiled, but I don't think anything react-scripts depends upon needs anything higher than node 4.

@cesarandreu

This comment has been minimized.

Copy link

cesarandreu commented Dec 19, 2016

I use include to decide what folders go through babel-loader. So far only one of my third-party dependencies is published using ES2015 code. My solution was to add it to that list:

{
    test: /\.js$/,
    include: [
      paths.app,
      path.resolve(paths.node_modules, 'cron-parser')
    ],
    loader: 'babel-loader',
    // ... other options
}

I think if you only care about node, setting up the tools and tranpsiling is actually a solid barrier, especially if you're unfamiliar with the overall ecosystem. First you need to get babel-cli and babel-preset-env. If you look at the docs you might ask yourself: should I enable that"loose" mode thing? Then, depending on what features you're using, you might need to grab babel-plugin-transform-runtime and babel-runtime, figure out what their options do, and which ones you might need to turn on / off. I don't think it's complicated once you're familiarized with the tooling, but there's still room for stumbling. Oh, and the end result usually means replacing the regular main field version with the harder to debug equivalent. :(

@julien-f

This comment has been minimized.

Copy link
Author

julien-f commented Dec 19, 2016

That's not so bad, maybe it should be exposed a nicer way in create-react-app.

@r3wt

This comment has been minimized.

Copy link

r3wt commented May 4, 2018

@gaearon how can i backport this to a crna app that has already been ejected. please let there be an easy button!
🙏

@franciscop-invast

This comment has been minimized.

Copy link

franciscop-invast commented Jul 12, 2018

The problem IMHO is that different people/companies have very different expectations for what is the baseline.

If you are supporting 1000s of devs for a "modern" npm library (as my own) then your baseline won't be ES5, it'll be ES6 already. If you are a company with millions (or billions) of users, or worse you are developing for Japan (as I do) then you definitely need to support IE and ES5. Same as if you use Electron, etc.

I normally do the export with UMD because I like being able to import, require and <script> for the different environments. But beyond that for my personal new projects I normally don't bother transpiling it. PR always welcome, but I expect this to start happening more and more.

When you have 10s or 100s of repositories/npm libraries, every extra, optional step counts towards maintainability and bandwidth.

Edit: so, thanks for adding support!

@Vadorequest

This comment has been minimized.

Copy link

Vadorequest commented Aug 30, 2018

Is there a solution for those who haven't ejected as of now?
I'm running into Failed to minify the code from this file today and discovering the whole thing, 2.0 version hasn't been released, I'm gonna try to update react-scripts but I doubt it'll fix it.

My issue was with ajv-error-messages, I forked and fixed it: https://github.com/StudyLink-fr/ajv-error-messages/

@KhalilMohammad

This comment has been minimized.

Copy link

KhalilMohammad commented Sep 13, 2018

Update react scripts, Latest version supports your scenario but there might be some problem as they are still in alpha version

@gaearon

This comment has been minimized.

Copy link
Member

gaearon commented Oct 2, 2018

@nerfologist

This comment has been minimized.

Copy link

nerfologist commented Oct 2, 2018

Hello, I'm experimenting with this new feature by substituting all of my named exports from lodash with named exports from lodash-es in a rather minimal application (although migrated from create-react-app 1.x).

The app starts just fine with create-react-app v2, but I am unable to run jest tests, as it's breaking on the es6 imports and exports:

  ● Test suite failed to run

    Jest encountered an unexpected token

    This usually means that you are trying to import a file which Jest cannot parse, e.g. it's not plain JavaScript.
   
    [...]

        Details:

    /Users/nerfologist/proj/newapp/node_modules/lodash-es/lodash.js:10
    export { default as add } from './add.js';
    ^^^^^^

    SyntaxError: Unexpected token export

      2 | import PropTypes from 'prop-types';
      3 | import qs from 'qs';
    > 4 | import { isEmpty } from 'lodash-es';
        | ^
      5 | import { Link } from 'react-router-dom';
      6 | import styled from 'styled-components';
      7 | import { translate } from 'react-i18next';

      at ScriptTransformer._transformAndBuildScript (node_modules/jest-runtime/build/script_transformer.js:403:17)
      at Object.<anonymous> (src/auth/ResetPasswordPage/ResetPasswordPage.js:4:1)

What is the correct way to address this? Should I add a custom Jest configuration file jest.config.js with a transformIgnorePatterns?

@Timer

This comment has been minimized.

Copy link
Collaborator

Timer commented Oct 2, 2018

@nerfologist please file a new issue

@facebook facebook locked as resolved and limited conversation to collaborators Oct 2, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.