fix(commonjs): Support JSX and TypeScript

[onigoetz]

Rollup Plugin Name: commonjs

This PR contains:

• bugfix
• feature
• refactor
• documentation
• other

Are tests included?

• yes (bugfixes and features will not be merged without tests)
• no

Breaking Changes?

• yes (breaking changes will not be merged unless absolutely necessary)
• no

If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.

List any relevant issue numbers: #622

Description

It is written in the documentation that the commonjs plugin goes before the babel plugin.
It works fine for most cases. But in some cases, like using TypeScript or JSX; this won't work as the parsing of the source will fail.
One workaround is to do the babel transpilation before, the risk with this approach though is that babel adds helpers (using import) and breaks the module detection of commonjs

The only safe approach would be to make sure that any syntax close enough to EcmaScript is parsed properly and re-output them identically; acorn-loose allows to do this.

Downside of this solution:

acorn-loose will make a valid AST even from the most broken code. So syntax that shouldn't be valid at all will parse correctly. Is it a problem ? in my opinion no, I think projects use linters or other tools to ensure their code is valid and this shouldn't happen too often.

Alternatives considered:

• Changing babel to issue helpers using commonjs syntax; looks complicated to do as this would require to change the TypeScript and swc logic as well
• Adding a first pass of babel to remove JSX, run commonjs, then babel again for the rest; Too complex to advertise, inefficient because the code is parsed again and again, also it won't work with TypeScript and SWC.

[onigoetz]

Hi @shellscape is there anything I can do to help this get merged ?
Thanks in advance

[shellscape]

I've requested review by some of the folks who know this plugin well.

[guybedford]

If you are executing commonjs with JSX embedded then you do want the Babel plugin to run before the commonjs plugin.

To avoid helper injection I would suggest only applying JSX transformation and nothing further.

You could even double-layer the Babel plugin.

I don't personally think it would be a good idea to parse CommonJS loosely - CommonJS is a well-defined module format with specific semantics. Changing them unnecessarily seems ill-defined to me.

[onigoetz]

Hi. I do understand your concern.

I'm afraid just transpiling JSX with babel won't work. As it will add it's own imports:

using https://babeljs.io/docs/en/babel-plugin-transform-react-jsx/ on

const profile = (
  <div>
    <img src="avatar.png" className="profile" />
    <h3>{[user.firstName, user.lastName].join(" ")}</h3>
  </div>
);

Will produce

import { jsx as _jsx } from "react/jsx-runtime";
import { jsxs as _jsxs } from "react/jsx-runtime";

const profile = _jsxs("div", {
  children: [
    _jsx("img", {
      src: "avatar.png",
      className: "profile",
    }),
    _jsx("h3", {
      children: [user.firstName, user.lastName].join(" "),
    }),
  ],
});

Which will also break the commonjs detection

I'm pretty sure that having some TypeScript input using enums or some more advanced features will also need some helpers to be transpiled enough to be parsed well by acorn.

double-layering the Babel plugin is one more level of parsing and that seems very inefficient anyway.

[shellscape]

Closing this one as it hasn't gotten any recent traction. I think the core team is in agreement with @guybedford that loose parsing isn't something we'd like to introduce into the commonjs plugin. However, there may be some room to introduce a third party plugin that does perform loose commonjs parsing. As there hasn't been a large call for that, a third party plugin (or a fork of commonjs) is probably the route to go. Thanks for your work on this and for your willingness to contribute to the project.

[onigoetz]

Thanks for the feedback