shelljs
Fix shelljs compatibility with esbuild bundler
Summary
Fixes https://github.com/shelljs/shelljs/issues/1160
- Add explicit file extension to dynamic command import.
- This call to require() is dynamic and does not add the file extension.
- Esbuild resolves files with their explicit file extension (eg './src/cat.js').
- Esbuild automatically adds .js extension in imports when not present but can't modify dynamic imports. For dynamic imports, file extension needs to be explicitly specified prior to bundling.
Test
Here is a script to reproduce (to be run in the repository root):
# Prepare
rm -rf test-esbuild && mkdir test-esbuild && cd test-esbuild
npm init --yes
echo "console.log(require('../shell.js').pwd().stdout && '\n OK');" > index.js
npm i -D esbuild
# Run
npx esbuild --platform=node --bundle index.js --outfile=out.js; node out.js
In the current master branch of this repository:
out.js 230.5kb
⚡ Done in 10ms
/.../shelljs/test-esbuild/out.js:5
throw new Error("Module not found in bundle: " + path);
^
Error: Module not found in bundle: ./src/cat
at /.../
Node.js v20.3.1
In this branch
out.js 230.5kb
⚡ Done in 9ms
OK
The bundle size doesn't change because all deps are already in the bundle, it's a resolution issue, as discussed in https://github.com/shelljs/shelljs/issues/1160
Have you tested this change on other bundlers? I'm not familiar with the current state of bundlers in the community. Is esbuild a popular one?
Have you tested this change on other bundlers?
I've tested:
- esbuild (37.5K stars, 31M weekly downloads)
- rollup (24.9K stars, 22.3M weekly downloads)
- @vercel/ncc (9K stars, 0.4M weekly downloads)
For esbuild, details are in the issue https://github.com/shelljs/shelljs/issues/1160
rollup
With rollup it doesn't work ouf of the box, but you can use a commonjs plugin and tweak its options so that these imports are treated dynamically (dynamicRequireTargets). This has minor security implications (Note: In extreme cases, this feature may result in some paths being rendered as absolute in the final bundle). This change doesn't affect rollup.
Reproducer
// rollup.config.js
var commonjs = require('@rollup/plugin-commonjs');
var nodeResolve = require('@rollup/plugin-node-resolve').nodeResolve;
module.exports = {
input: 'index.js',
output: {
file: 'out.js',
format: 'cjs',
},
plugins: [commonjs({
dynamicRequireTargets: [
'node_modules/shelljs/src/*.js',
],
}), nodeResolve()],
};
// index.js
var shelljs = require('shelljs');
console.log(shelljs.pwd().stdout && '\n OK');
// bash
//$ npx rollup -c rollup.config.js
ncc
With ncc it works out of the box because ncc assumes the require could be called without the .js extension as well. (for our use-case, ncc also creates a separate exec-child.js file which isn't suitable for us). This change doesn't affect ncc.
Reproducer
npx ncc build index.js -o out
node out/index.js
Would also love to get this in.shelljs is very useful for github actions
I am also highly interested in this, shelljs would be even more useful if it could be bundled! @nfischer
Currently also facing the ./src/cat issue which is so often reported in the issues of this project.
I tried the fix proposed here, but it did not work in my setup with esbuild. However, just manually writing the require statements works for me (even though it is not so elegant):
https://github.com/AlexanderLill/shelljs/pull/1
I have the same issue. It's very common to use esbuild to bundle VSCode extensions. Is there any plan to merge it? Thanks!
I'm willing to revisit this, however I need someone to show me how to test this change to make sure it actually works. I also see #1119 was raised for a similar issue, so I'm not sure which is the better approach.
Hello @nfischer , thanks for answering!
I'm not sure which is the better approach.
As a general rule of thumb, I say dynamic imports is something to avoid because that will always mess with build-time tree shaking. Bundlers can cope with this but this relies on blindly putting all the file content in the bundle.
TBH moving to static imports would be the preferred approach if it was my project, but I also think my change is fine if you want to keep the dynamic import.
however I need someone to show me how to test this change to make sure it actually works
For testing, I've put there a reproducer for ESBuild, do you want a reproducer for other bundlers?