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

It should be clarified that plugin-commonjs should be placed before plugin-babel #622

Closed
1 of 4 tasks
shrinktofit opened this issue Oct 24, 2020 · 8 comments
Closed
1 of 4 tasks

Comments

@shrinktofit
Copy link

  • Rollup Plugin Name: plugin-commonjs/plugin-babel
  • Rollup Plugin Version: latest

Documentation Is:

  • Missing
  • Needed
  • Confusing
  • Not Sure?

Please Explain in Detail...

Since how @rollup/plugin-commonjs and @rollup/plugin-babel are implemented. It's better to hint user that when specify rollup plugins, they'd better place @rollup/plugin-commonjs before @rollup/plugin-babel.

Reason

Given a module in CommonJS format and its code needs to be processed by babel. For example:

module.exports = typeof window;

@rollup/plugin-babel will inject helper codes:

import { typeof as _typeof } from "\0rollupPluginBabelHelpers.js";
module.exports = (typeof window === "undefined" ? "undefined" : _typeof(window));

then, the transformed source will be passed to @rollup/plugin-commonjs. The CommonJS plugin then treat the source as ES6 module(because of the import syntax @rollup/plugin-babel inserted) and does not process further and leads to *** is not exported by ***.

Your Proposal for Changes

Document the order issue in plugin section.

@shellscape
Copy link
Collaborator

Thanks for the issue. Happy to review a PR which adds this to the README.

@jbt
Copy link

jbt commented Nov 28, 2020

I came across this issue while running into problems with commonjs & babel - I was actually running into a similar issue but I found that often it doesn't work whichever way around you have the plugins.

For example, if you have syntax like JSX (which the commonjs plugin can't parse), you have to put babel before commonjs. But if your commonjs code requires things like the typeof helper reported above, then commonjs has to come before babel.

I've ended up having to include the babel plugin twice in my config (once before commonjs, once after) to make this work, which feels hacky.

Perhaps the circular-dependency between the two plugins is a bigger thing that needs addressing, I'm happy to try and summarise that in a new issue. But based on my experience it doesn't seem as simple as saying one plugin or the other ought to be placed first

@stale stale bot added the x⁷ ⋅ stale label Jan 27, 2021
@stale
Copy link

stale bot commented Jan 28, 2021

Hey folks. This issue hasn't received any traction for 60 days, so we're going to close this for housekeeping. If this is still an ongoing issue, please do consider contributing a Pull Request to resolve it. Further discussion is always welcome even with the issue closed. If anything actionable is posted in the comments, we'll consider reopening it.

@shellscape
Copy link
Collaborator

resolved in 6b4b7b6

@teehemkay
Copy link

resolved in 6b4b7b6

IMHO the recommandation should be further contextualized and more nuanced as pointed out by @jbt above and @Andarist in #805.

The order suggested is simply wrong in the case of jsx files where @rollup/plugin-babel MUST come before the @rollup/plugin-commonjs otherwise rollup errors out with a SyntaxError because it can't parse the jsx syntax.

@Andarist
Copy link
Member

Andarist commented Aug 5, 2021

@teehemkay feel free to prepare a PR that would clarify this further

@teehemkay
Copy link

teehemkay commented Aug 5, 2021

@jbt

... I'm happy to try and summarise that in a new issue. But based on my experience it doesn't seem as simple as saying one plugin or the other ought to be placed first

Would you please consider creating this issue? You seem to have explored it further than I have since I only dealt with the specific issue of jsx syntax. Then I'd gladly prepare an PR for the documentation pointing to the new issue.

@onigoetz
Copy link

onigoetz commented Aug 7, 2021

I stumbled on the same problem just now it seems that there are many things at play here.

  • As @jbt explained one possible solution would be to use babel twice, but that seems VERY inefficient. parsing is costly, and re-parsing a file at least three times (babel x2, commonjs x1) seems sub-optimal.
  • What about TypeScript? I doubt that rollup's parser will be happy with it, so the problem will most likely be the same here.

So that brings me to a few possible solutions

  1. Improve Rollup's parser to support both TypeScript and JSX, but that means that every imaginable syntax would have to be supported
  2. Make sure that babel/typescript/swc's helpers are added with the right syntax (cjs or es) according to what the file already has (Pretty sure babel can't do that)
  3. Is it possible to have a "permissive parser" that would be able to parse the top level parts and ignore the failures ?
  4. Write plugin commonjs as a babel plugin, not sure if that's possible or if it maybe already exists (wouldn't cover typescript or swc though)

I'm no Rollup/babel expert though and have no good idea on the feasability of each of these ideas

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants