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

[Feature Request]: Support transforming CJS require calls into ESM imports #2041

Open
AdrianGonz97 opened this issue Aug 22, 2024 · 5 comments

Comments

@AdrianGonz97
Copy link

What problem does this feature solve?

Rolldown's current internal implementation of @rollup/plugin-commonjs is seemingly missing the feature of transforming CJS require calls into ESM imports when outputting to an esm format.

At the moment, the require calls remain in place, making it impossible to run the outputted code in node when the package.json type is set to type: "module":

➜  node dist-rolldown                            
file:///home/koala/Reproductions/rolldown-missing-transformation/dist-rolldown/index.js:28
        const path = require("node:path");
                     ^

ReferenceError: require is not defined in ES module scope, you can use import instead
This file is being treated as an ES module because it has a '.js' file extension and '/home/koala/Reproductions/rolldown-missing-transformation/package.json' contains "type": "module". To treat it as a CommonJS script, rename it to use the '.cjs' file extension.
    at file:///home/koala/Reproductions/rolldown-missing-transformation/dist-rolldown/index.js:28:15
    at file:///home/koala/Reproductions/rolldown-missing-transformation/dist-rolldown/index.js:9:48
    at file:///home/koala/Reproductions/rolldown-missing-transformation/dist-rolldown/index.js:35:31
    at ModuleJob.run (node:internal/modules/esm/module_job:234:25)
    at async ModuleLoader.import (node:internal/modules/esm/loader:473:24)
    at async asyncRunEntryPointWithESMLoader (node:internal/modules/run_main:123:5)

Node.js v20.17.0

What does the proposed API look like?

Reproduction: https://github.com/AdrianGonz97/rolldown-missing-cjs-transformation.git

Input:

// entry.ts
import { foo } from "./commonjs.cjs";

console.log(foo);
// commonjs.cjs
const path = require("path");

const foo = path.resolve(".");

module.exports = { foo };

Output:

Rolldown (fails to execute in type: "module" mode):

//#region rolldown:runtime
var __create = Object.create;
var __defProp = Object.defineProperty;
var __getOwnPropDesc = Object.getOwnPropertyDescriptor;
var __getOwnPropNames = Object.getOwnPropertyNames;
var __getProtoOf = Object.getPrototypeOf;
var __hasOwnProp = Object.prototype.hasOwnProperty;
var __commonJSMin = (cb, mod) => () => (mod || cb((mod = {exports: {}}).exports, mod), mod.exports);
var __copyProps = (to, from, except, desc) => {
	if (from && typeof from === "object" || typeof from === "function") for (var keys = __getOwnPropNames(from), i = 0, n = keys.length, key; i < n; i++) {
		key = keys[i];
		if (!__hasOwnProp.call(to, key) && key !== except) __defProp(to, key, {
			get: ((k) => from[k]).bind(null, key),
			enumerable: !(desc = __getOwnPropDesc(from, key)) || desc.enumerable
		});
	}
	return to;
};
var __toESM = (mod, isNodeMode, target) => (target = mod != null ? __create(__getProtoOf(mod)) : {}, __copyProps(isNodeMode || !mod || !mod.__esModule ? __defProp(target, "default", {
	value: mod,
	enumerable: true
}) : target, mod));

//#endregion
//#region commonjs.cjs
var require_commonjs = __commonJSMin((exports, module) => {
	const path = require("node:path");
	const foo$1 = path.resolve(".");
	module.exports = {foo: foo$1};
});

//#endregion
//#region entry.js
var import_commonjs = __toESM(require_commonjs());
console.log(import_commonjs.foo);

//#endregion

Rollup's @rollup/plugin-commonjs plugin (works as expected):

import require$$0 from 'path';

const path = require$$0;

const foo = path.resolve(".");

var commonjs = { foo };

console.log(commonjs.foo);
@underfin
Copy link
Contributor

The vite extenal tests also has the problem, see here. It need to convert require to import and using importmap load it.

@hyf0
Copy link
Member

hyf0 commented Aug 23, 2024

Since we are porting esbuild, esbuild also have this behavior and explain the reason in

We met this also in rolldown's self-bootstrapping and used workaround like this

banner: [
`import __node_module__ from 'node:module';`,
`const require = __node_module__.createRequire(import.meta.url)`,
].join('\n'),

Transforming require('ext') to import 'ext' will certainly breaks the semantic:

  • imported modules has side-effects
  • module graph's execution order

@benmccann
Copy link

benmccann commented Aug 23, 2024

Yeah, I've contributed a fair amount to Vite and unfortunately this case is hit in almost all realworld projects. Only about 20% of libraries distribute ESM code, so as soon as you add a dependency to your project, there's a good chance you're hitting this and in a real project it's almost unavoidable

@hyf0
Copy link
Member

hyf0 commented Aug 24, 2024

Some cases:

  • import 'ext' and require('ext') will fall into different resolve logic of node

@hyf0
Copy link
Member

hyf0 commented Aug 24, 2024

Since we have a platform: node option, I guess it's ok rolldown support injecting automatically

banner: [
`import __node_module__ from 'node:module';`,
`const require = __node_module__.createRequire(import.meta.url)`,
].join('\n'),

if rolldown detects require while targeting esm output.

But transforming require into import is another matter.

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

4 participants