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

Discuss .babelrc detection, engines field, etc. #13

Closed
devongovett opened this issue Dec 5, 2017 · 107 comments
Closed

Discuss .babelrc detection, engines field, etc. #13

devongovett opened this issue Dec 5, 2017 · 107 comments
Labels
💬 RFC Request For Comments

Comments

@devongovett
Copy link
Member

Based on twitter convo:

https://twitter.com/dan_abramov/status/938090483166924800

I’d suggest to reconsider this (or at least limit it to compiling with -env preset only). This encourages authors to publish packages with stage 0 proposals, and ultimately makes builds slower for everyone. Also .babelrc won’t be compatible between Babel versions.

@mswiecicki
Copy link

As i understand the fundamental issue here is to detect how much transpilation (if any) is needed for each third party dependency, correct?

Stop me if it's stupid but couldn't we somehow check package.jsons of dependencies?
I mean is there really any other way to programatically check what is needed by given project?
Though i imagine it could be potentially costly... :/

@benjamn
Copy link

benjamn commented Dec 6, 2017

Whether or not to honor .babelrc files in nested node_modules directories is a hot topic over at the Babel repo, too: babel/babel#6766

In short, most npm packages are fully compiled before they are published, so it is wasteful/inappropriate to recompile them (even using their own .babelrc files). It would be great if those packages put .babelrc in their .npmignore files, but that doesn't always happen.

For the minority of npm packages that do need to be recompiled for the browser, the application developer needs to be able to intervene somehow, to enable recompilation selectively. I can imagine various ways of configuring that behavior, but it may be hard/impossible to make the right choices without increasing the configuration burden somewhat.

Taking a step back: Parcel is competing with Webpack, which is known for using configuration to solve any and all problems. While "zero-configuration" may be more of a goal than a reality (you still have to configure packagers, e.g.), I think it's a really valuable goal, so it's worth thinking hard about how to solve this problem with minimal additional configuration.

@mswiecicki
Copy link

mswiecicki commented Dec 6, 2017

@benjamn So do I understand correctly that the issue lies in whether it is necessary to recompile given dependency and if so using which .babelrc? Because if that's the case maybe we could choose the default behaviour, and allow the other via some CLI flag?

It does seem like a band-aid of some kind though...

@jamiebuilds
Copy link
Member

I'm generally against doing any kind of transforms on node_modules.

Basing things off of package.json#engines#node and only using babel-preset-env would be an improvement, but we'd still have the problem of unsupported APIs in older versions of Node.

I suppose if someone shipped:

{
  "engines": {
    "ecmascript": "2017"
  }
}

We could deal with that a bit easier since we can polyfill and transform all the relevant bits.

I'm still not convinced that's a good idea.

@spalger spalger mentioned this issue Dec 7, 2017
@davidnagli
Copy link
Contributor

davidnagli commented Dec 7, 2017

Does Babel currently transpile node_modules?

@jamiebuilds
Copy link
Member

@davidnagli Babel itself defaults to ignoring node_modules. Parcel (currently) undoes that behavior and will run it on node_modules if it finds a .babelrc

@chipit24
Copy link

chipit24 commented Dec 8, 2017

This is currently causing some issues for me. All the 3rd party modules I import (from node_modules) for my application are already compiled/transformed; I don't want Parcel to run any transofrms even if it finds a .babelrc file there.

@davidnagli
Copy link
Contributor

davidnagli commented Dec 8, 2017

@chipit24 I guess we’re gonna have to work on resolving this issue. In the meantime, if you’re looking for a workaround use the --no-minify flag

@tjenkinson
Copy link

I've always understood that the 'main' property in package.json should point to an es5 build, so transpilation should not be necessary. Is this not the correct assumption?

@davidnagli davidnagli added the 💬 RFC Request For Comments label Dec 11, 2017
@chipit24
Copy link

There's no real requirement for what the main field should point to. There's some interesting info in the rollup.js docs:

Yes. If you're using things like arrow functions and classes (or more exotic features like decorators and object spread), you should transpile them if you want your library to work in environments that don't support those features. Otherwise, whoever uses your library will have to transpile it themselves, including configuring their transpiler to handle your code, which might involve esoteric transforms and conflicting versions of Babel.

It's a frequent source of frustration. Be a responsible library author and ship code that actually runs in the environments you support!

@tjenkinson
Copy link

I see. My opinion is that parcel should see packages as read only and not run any transformations on content in them unless this is explicit in some way.

@govizlora
Copy link

@davidnagli Could you specify how to use the --no-minify flag? Maybe I'm missing something but I just want to find a workaround for babel to ignore node_modules. Thanks a lot!

@rj254
Copy link

rj254 commented Dec 13, 2017

Also, a common problem is that people accidentally publish their node_modules with .babelrc even if it is already transpiled.

Additionally, let us say we have package X which uses babel-preset-stage-1, which includes a .babelrc for some reason. Our project includes package X, but uses babel-preset-env. The current behavior of attempting to transpile package X again results in a Couldn't find preset stage-1 relative to directory "PATH_TO_X". Because in most cases, package X will install babel-preset-stage-1 as a devDependency, it will not be installed when running yarn (probably npm install) on our project.

I think that a flag to disable this behavior would be very helpful (or have this behavior prevented by default and allow it to be toggled on). Also, I'm not quite sure what the --no-minify would solve this issue.

@Cinamonas
Copy link

It would be great to have an option to bypass .babelrc detection in node_modules that are being imported into the project.

I've created a minimal example of it breaking the build: https://github.com/Cinamonas/parcel-babelrc-issue

Is there currently any workaround? Because if you do import a transpiled module that also includes .babelrc (probably accidentally), you hit a wall.

@codyhatch
Copy link

Any progress on a workaround? This seems like a pretty critical issue which makes parcel more-or-less unusable in many situations.

To be clear, my issue is:

  1. I have a small project that uses a few npm libraries
  2. Some of them have included .babelrc files despite already having properly transpiled output in the npm package, which is unfortunate but common
  3. Parcel is trying to recompile them using Babel plugins which are not installed, and which should not be installed, and which really shouldn't be used even if they were installed.

One (of many!) examples is the react-slick package, which yields an error like this for a project that uses it:

foo/node_modules/react-slick/lib/index.js: Unknown plugin "transform-object-assign" specified in "foo/x/node_modules/react-slick/.babelrc" at 0, attempted to resolve relative to "foo/node_modules/react-slick"

It has a .babelrc file, but it also includes the already compiled output; no transpiling is needed.

@shawwn
Copy link
Contributor

shawwn commented Dec 21, 2017 via email

@chipit24
Copy link

chipit24 commented Dec 21, 2017

Even if we can determine whether or not the library has been transpiled, it may not be desirable to simply apply the transforms described in said library's babel config. Aside from the clear problem of conflicting babel versions and missing babel plugins, there's other things to consider.

For example, let's say parcel can detect transpiled code and conditionally run transforms if the code isn't transpiled. I'm building a package that pulls in package X which parcel knows has not been transpiled, but the babel config for this package only transpiles code to ES2015 (ES6). I want my code to support the ES5 spec - so this solution won't suffice.

There appears to be a solution for the above issue with webpack, via https://github.com/andersdjohnson/webpack-babel-env-deps (which I discovered through the discussion at babel/babel-loader#171); so perhaps we can take some ideas from there.

@gaearon
Copy link

gaearon commented Jan 7, 2018

IMO it is fundamentally wrong to try to respect .babelrc in other projects. This means that if I publish a library using Babel 6 but Parcel updates to Babel 7, my library is suddenly not useable anymore. As a publisher I don’t want my consumers to depend on Babel at all, much less to be confined to a single major because I accidentally included a config file in the package.

@devongovett
Copy link
Member Author

My comments from twitter:

I totally agree that there should be a more standard way of publishing non-ES5 code to npm, but I also think it’s important to let modules specify their own custom transforms as well which only apply locally.

We should not distinguish between app code and module code - they should build the exact same way. If you imported another app from node_modules, it should build the same way as if it were an entry point.

We should use the version of Babel specified in the module not the one in the app. That way it will always build as the module intended.

@devongovett
Copy link
Member Author

To elaborate:

  1. We already do not transpile with babel unless a .babelrc file is found. We definitely do not want to run babel over all node_modules.
  2. node_modules should build exactly the same way if they are the entry point or a dependency of another app. e.g. parcel build module.js should produce the same output for module.js as parcel build app.js which depends on node_modules/module/module.js. If module.js needs to compile some things with babel, that needs to be part of the build process.
  3. Transpiling things pre-publish to npm is not a good solution. Ideally, the source code would be transpiled based on a browser support matrix defined in the app, which may not be all the way down to ES5. Also, it is very useful to be able to npm link a module locally and just edit the source files, without needing to have a manual build step. This is only possible if Parcel handles the transpilation for you.
  4. We should not treat JS differently from other languages. If you have a TypeScript file in your node_modules, we transpile it with the locally specified version of the typescript compiler. Same for SASS, CoffeeScript, etc. JS should be no different. If we use the locally specified babel version, this should work as desired.
  5. The main issue here is that modules are published to npm with .babelrc by mistake, but are already pre-transpiled. This could be solved in a number of ways: we could require an additional flag to enable babel, we could detect if the babel is actually specified as a dependency (not devDependency), etc. Not sure any of those are really ideal, but we do need to do something to work around this issue.

I think we should go for a two pronged approach:

  1. We support some way to specify the version of JavaScript that a module is written against. This could be something like the engines field in package.json: "engines": { "ecmaVersion": "ES2015" }. This would enable transpilation. The target would be specified by the app rather than an individual module via a .browserslistrc file or similar.
  2. We support an explicit way to turn on babel for node_modules, to enable custom transforms. Browserify does this via transform field in package.json. Perhaps we can come up with some similar mechanism. This is not ideal as it breaks the "zero-configuration" promise, so if there is a better way to detect when NOT to run babel when there is a .babelrc, I'm all ears.

In any case, we need a lot more community feedback on how do implement this, along with support from other bundlers, before we jump into this. I don't want to come up with something Parcel specific here.

@youknowriad
Copy link

youknowriad commented Jan 7, 2018

In any case, we need a lot more community feedback on how do implement this, along with support from other bundlers, before we jump into this. I don't want to come up with something Parcel specific here.

I understand the reasons behind both positions (transpiling or not) and I like this ⬆️, trying to come with a "standard" across all npm modules.


That said, this will likely take a lot of time to achieve something decent and in the meantime refrain people from using parcel broadly because packages they're using with their current setup (webpack) can't be used with parcel. Do you think coming up with a temporary workaround (a .parcelignore or similar) could be an option?

@emartini
Copy link

emartini commented Feb 9, 2018

@jsiebern if the babel config is in the package.json it won't be enough :(

@devongovett
Copy link
Member Author

Ok merged #559, which disables .babelrc on node_modules and enables automatic compilation with babel-preset-env for modules that define a higher browser/node version requirement in browserslist or engines.node than the app's target. Should fix this issue for most people. I'm still interested in a potential "source" field in package.json, so leaving this issue open to discuss that.

@spion
Copy link

spion commented Feb 28, 2018

This is great. I was thinking, maybe a general --conservative switch would be nice, where parcel prefers not compiling external modules over compiling them as much as possible.

For some reason, libraries don't seem to care that they're publishing broken modules as long as their "main" field is correct, and it doesn't seem like parceljs can exert sufficient pressure to prioritise fixing those breakages e.g. see palantir/blueprint#2144

Got mixed feelings about this though, since in cases like these its absolutely clear that the library is broken and parcel is helpful pointing that out. Unfortunately app developers are the ones getting the helpful info, not library developers.

Maybe a parcel-sponsored linter that detects common issues and that library users could drop into their project would be helpful? A "ready-for-the-future" linter that e.g. detects incorrectly written es6 modules among other things.

@tj
Copy link

tj commented Mar 5, 2018

I generally agree with everyone mentioning that node_modules should be left as-is, however, I think maybe detecting links and compiling those makes sense? I can't imagine symlinking non-dev projects often, so treating it as something you're actively developing makes sense to me at least!

@luke-robertson
Copy link

Is this fixed ? i think im going to have to go back to webpack

@DeMoorJasper
Copy link
Member

@prependto It has been fixed and released with v1.6

@Vanuan
Copy link

Vanuan commented Mar 14, 2018

It's fixed, but now the problem is how to force babel compilation for modules that need to be compiled.

For example, it's common for monorepo to symlink source packages to node_modules (#948).

@devongovett
Copy link
Member Author

Just opened #1101 which adds support for a source field to enable compilation of symlinked modules. Would be curious to hear your feedback!

@devongovett
Copy link
Member Author

Closing since #1101 is merged.

@rodoabad
Copy link

Is there going to be support for non-linked modules? i.e. transpiling from node_modules if the packages are not linked?

@cyrilchapon
Copy link

I came here after

#1128
babel/babel#4125
facebook/create-react-app#5267

In here we're using react-map-gl (which uses mapbox-gl), and we're getting a runtime error

Uncaught ReferenceError: _typeof is not defined
(⚠️ coming from a WebWorker)

Which apparently comes - from what I understood - when _typeof from @babel/helper is not being included inside web-workers.

I'm posting here, because everyone seems to be saying that

node_modules packages are not transpiled.

I'm clearly encountering that issue because one of those 2 packages is transpiled...
I can prove that because nothing talks about _typeof inside the whole react-map-gl neither mapbox-gl package... I know there is an underlying issue that should be resolved, and I'll ping on the appropriate issue for this, but for now I'm looking for a workaround (i.e just disable transpiling completely of those 2 modules)

I'm very lost here... they aren't supposed to be built, but they are, apparently.

Some additional infos :

  • none of (npm installed versions of) those packages have a babelrc
  • none of the those packages have a babel config in package.json
  • react-map-gl exposes inside package.json :
      "main": "dist/es5/index.js",
      "module": "dist/esm/index.js",
    
  • mapbox-gl exposes inside package.json :
      "esm": true,
      "files": [
        "build/",
        "dist/",
        "flow-typed/",
        "src/",
        ".flowconfig"
       ],
      ...
      "main": "dist/mapbox-gl.js",
    

What is the truth ? Are they supposed to be transpiled or not ? Why ?

Thanks for your help...

@louh
Copy link

louh commented May 7, 2019

Hey! Coming here after spending half a day in frustration. I'm reasonably certain I know what the problem is, but not how to solve it. Right now, I have a dependency (https://github.com/yahoo/react-dnd-touch-backend/) that publishes its own .babelrc, although it really doesn't need to. In my own project, this results in Duplicate preset/plugin detected error. If I manually remove .babelrc, it's fine. But if I leave it in, I have no idea how to work around it. Everything I can think of feels like a bad workaround.

I can wait for the library author to unpublish .babelrc, but that might be the only good option at this point, and meanwhile my collaborators cannot actually do their work...

@louh
Copy link

louh commented Jul 2, 2019

Just wanted to update comment here and note that the upstream package maintainer did remove .babelrc! 👏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💬 RFC Request For Comments
Projects
None yet
Development

No branches or pull requests