-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Rewrite require()
calls of Node built-ins to import statements when emitting ESM for Node
#2067
Conversation
require()
calls of Node built-ins to import statements when targeting Noderequire()
calls of Node built-ins to import statements when emitting ESM for Node
This seems to work quite well, however when trying to use esbuild targeting esm with I guess it will continue to be a hassle to have code-splitting in a node application using esbuild as long as we have to rely on esm while tons of libraries not being ready for esm. On a side note. The plugin I was using to fix import type { Loader, Plugin } from 'esbuild';
import fs from 'fs-extra';
export const esmPlugin = (): Plugin => ({
name: 'esmplugin',
setup(build) {
build.onLoad({ filter: /.\.(js|ts|jsx|tsx)$/, namespace: 'file' }, async (args) => {
const globalsRegex = /__(?=(filename|dirname))/g;
let fileContent = new TextDecoder().decode(await fs.readFile(args.path));
let variables = fileContent.match(globalsRegex)
? `
import { fileURLToPath as urlESMPluginFileURLToPath } from "url";
import { dirname as pathESMPluginDirname} from "path";
var __filename =urlESMPluginFileURLToPath(import.meta.url);
var __dirname = pathESMPluginDirname(urlESMPluginFileURLToPath(import.meta.url));
`
: '';
const contents = variables + '\n' + fileContent;
const loader = args.path.split('.').pop() as Loader;
return {
contents,
loader,
};
});
},
}); now fails with this esbuild build, where it didn't do that before:
Not sure how keep the behavior from working... |
@ghaedi1993 can you merge it? |
hi @evanw , plase merge is pr if it will helpful the issue |
Excellent work! Hope to see this merged. |
running into this exact issue so would appreciate the merge :) |
Great work! Any chance we can see this merged? |
Can we have some light on what is needed for this to be merged? |
@evanw please |
Maybe something like below, when target import {createRequire} from 'module'
var __require = requireWrapper(createRequire(import.meta.url)) Fix all |
It is possible to work around this for the time being. cc @eduardoboucas The following approach lets us get working ESM bundles out. It requires In the emitted bundle, esbuild adds a polyfill to check if there's a This requires ApproachSee the const ESM_REQUIRE_SHIM = `
await (async () => {
const { dirname } = await import("path");
const { fileURLToPath } = await import("url");
/**
* Shim entry-point related paths.
*/
if (typeof globalThis.__filename === "undefined") {
globalThis.__filename = fileURLToPath(import.meta.url);
}
if (typeof globalThis.__dirname === "undefined") {
globalThis.__dirname = dirname(globalThis.__filename);
}
/**
* Shim require if needed.
*/
if (typeof globalThis.require === "undefined") {
const { default: module } = await import("module");
globalThis.require = module.createRequire(import.meta.url);
}
})();
`;
/** Whether or not you're bundling. */
const bundle = true;
/** Tell esbuild to add the shim to emitted JS. */
const shimBanner = {
"js": ESM_REQUIRE_SHIM
};
/**
* ESNext + ESM, bundle: true, and require() shim in banner.
*/
const buildOptions: BuildOptions = {
...common,
format: "esm",
target: "esnext",
platform: "node",
banner: bundle ? shimBanner : undefined,
bundle,
};
esbuild(buildOptions); For a minified version of the shim, you can use the following (in general, you should not add minified code to the top of your bundle contexts because a stranger on GitHub tells you to, but feel free to verify it): const ESM_REQUIRE_SHIM = `
await(async()=>{let{dirname:e}=await import("path"),{fileURLToPath:i}=await import("url");if(typeof globalThis.__filename>"u"&&(globalThis.__filename=i(import.meta.url)),typeof globalThis.__dirname>"u"&&(globalThis.__dirname=e(globalThis.__filename)),typeof globalThis.require>"u"){let{default:a}=await import("module");globalThis.require=a.createRequire(import.meta.url)}})();
`; The end result of these build options are a single ESM bundle with all non-builtin modules inlined, and a global FootnotesIf your program depends on |
@ctjlewis This looks like an interesting approach that might even fix the issues I had with prisma. However, how does esbuild know the modules required with 'require' should be part of the bundle? Doesn't esbuild follow import statements to understand what is part of the bundle? |
You need |
contents: `const { extname } = require("path"); export { extname };`, | ||
}, | ||
bundle: true, | ||
bundle: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you want two bundle
properties here?
Anything we can do to help move this along? This is an issue that's also affecting a number of libraries we maintain, and as more and more of the ecosystem is moving to ESM this appears to be picking up steam. |
// This needs to use relative paths to avoid breaking on Windows. | ||
// Importing by absolute path doesn't work on Windows in node. | ||
const result = await import('./' + path.relative(__dirname, outfile)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: have you considered using url.pathToFileURL
? Something along the following lines:
// This needs to use relative paths to avoid breaking on Windows. | |
// Importing by absolute path doesn't work on Windows in node. | |
const result = await import('./' + path.relative(__dirname, outfile)) | |
const result = await import(url.pathToFileURL(outfile)) |
Quoting from nodejs/node#34765:
This behavior isn't restricted to windows paths, the same is true for absolute Unix paths as well. The module import specifiers only support URLs, not file paths. To import file paths, they need to be converted to URLs first on all platforms (like you mentioned).
The general problem with supporting both URLs and file paths is that some URLs are also valid (but different!) file paths and vice versa. We decided to go with URLs over file paths to be consistent with other runtimes (e.g. browsers) that are also using URL-based import specifiers.
This work for me: const buildOptions: BuildOptions = {
...common,
format: "esm",
target: "esnext",
platform: "node",
banner: 'import { createRequire } from 'module';const require = createRequire(import.meta.url);',
bundle,
};
esbuild(buildOptions);` |
[0] file:///Users/josuevalencia/dog/the-dog/ui/etc/esbuild/esbuild.mjs:62 |
Maybe I'm missing something, but is there a reason this hasn't been merged?? Seems like a pretty substantial bug, has been around for months and months, already has the PR ready, and has community support. Does anyone have insight onto why this bug still exists? |
Revokes the license of @revmischa per evanw/esbuild#2067.
Checks that `process` is not falsy rather than checking if `document` is truthy to determine if Node context. Does not use redundant IIFE. cc evanw/esbuild#2067
const buildOptions: BuildOptions = {
...common,
format: "esm",
target: "esnext",
platform: "node",
banner: {
js: "import { createRequire } from 'module'; const require = createRequire(import.meta.url);",
},
bundle,
};
esbuild(buildOptions); |
Closing, as it got stale and the community seems to have found different workarounds. |
I understand that there is a way around this, but this issue still affects the community. It is hard to find this hack workaround as the error is quite vague and simply searching for a solution does not lead to this issue. This leads to people wasting time. The initially proposed solution seemed a bit more elegant. Would it be possible to reconsider a proper implementation for this issue? If you do decide that no it's not worth it, would it be possible to document that the banner hack approach is mandatory when bundling as esm? thanks |
The purpose of |
I’m reopening this because I still plan on doing this. |
Ah whoops sorry, didn’t see that this was a PR instead of an issue. I’ll close again. In any case, I still plan on doing this. |
@ctjlewis by depends on esbuild, do you mean you bundle with esbuild, or you use esbuild as a module in your app? |
Node workaround per: evanw/esbuild#2067
@evanw did you get around to this? |
workaround above works for
|
I mean, "if your program contains an |
Re this issue: The burden of adding this polyfill by default would probably cause Evan more headaches than letting us polyfill ourselves, so it looks like it's been shelved. To get this working, ensure there's a |
Is it possible to add this shim by default when target to Node env and format is ESM at least ? |
When outputting ESM, any
require
calls will be replaced with the__require
shim, sincerequire
will not be available. However, since external paths will not be bundled, they will generate a runtime error.This is particularly problematic when targeting Node, since any Node built-in modules are automatically marked as external and therefore requiring them will fail. This is described in #1921.
That case is addressed in this PR. When targeting Node, any
require
calls for Node built-ins will be replaced by import statements, since we know those are available.Example
Take the example below:
And the following command to bundle it with esbuild:
Before 🔴
Without this PR, esbuild produces the following bundle:
Running the bundle generates a runtime error:
After ✅
With this PR, esbuild produces the following bundle:
And running the bundle produces the expected output:
Notes
This issue impacted Netlify customers, as reported in netlify/zip-it-and-ship-it#1036. We would love to contribute a fix back to esbuild, and we'll be happy to accommodate any feedback from @evanw and the esbuild community into our PR.
If the maintainers decide against merging this functionality, we'll probably add it to our esbuild fork (which you can read about here). For this reason, I've tried to reduce the surface area of the changes to the absolute minimum, which reflects in a couple of implementation details:
Passing the list of Node built-in modules from the bundler to the parser feels a bit awkward. This is due to the fact that importing the resolver from the parser would lead to an import cycle. To avoid moving the list of Node built-ins to its own package and include from the resolver and the parser, I'm passing the list into the parser.
The
__require
runtime shim is not being dead-code-eliminated when it's not used, as shown in the example above. We can address this in different ways, depending on what the maintainers decide to do with this PR.