Explicitly require commands #1119
Conversation
|
I'd love to see this merged - is there a reason not to? |
|
I suggested another solution to this issue in here, but I've only tested it with |
|
It would apply to every bundler @7PH |
|
Later, I've tested more bundlers and some already work with But my PR isn't getting a lot of attention by the owner of |
|
Dear maintainers @nfischer @freitagbr, please consider merging this minor change to make shelljs compatible with esbuild and other bundlers (see previous comments and many of the open issues, e.g. #1160 #1172 etc). I am currently using my own fork of this repository to fix this, and I think many people would benefit from this change. |
| require('./src/echo'); | ||
| require('./src/error'); | ||
| require('./src/errorCode'); | ||
| // require('./src/exec-child'); excluded since it is for commandline only |
There was a problem hiding this comment.
Can we remove this line? This is just a code comment so it doesn't do anything in regular node. Is this commented-out code necessary in order to trick the bundler?
There was a problem hiding this comment.
I think you should change this to just actually require() the exec-child.js file. If you rebase on the lastest master branch, then I have changed this to be a NOOP. I think requiring is necessary to trick the bundler into realizing this file is necessary.
|
No, I thought it was nice for the sake of completeness of all the files in
./src, but I can remove it.
…On Wed, Feb 19, 2025, 11:20 p.m. Nate Fischer ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In shell.js
<#1119 (comment)>:
> @@ -21,9 +21,40 @@ var common = require('./src/common');
***@***.***
// Load all default commands
-require('./commands').forEach(function (command) {
- require('./src/' + command);
-});
+require('./src/cat');
+require('./src/cd');
+require('./src/chmod');
+require('./src/cmd');
+require('./src/common');
+require('./src/cp');
+require('./src/dirs');
+require('./src/echo');
+require('./src/error');
+require('./src/errorCode');
+// require('./src/exec-child'); excluded since it is for commandline only
Can we remove this line? This is just a code comment so it doesn't do
anything in regular node. Is this commented-out code necessary in order to
trick the bundler?
—
Reply to this email directly, view it on GitHub
<#1119 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABZV3OZALOID7MN2EAPNODT2QV62LAVCNFSM6AAAAAAVW64P42VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDMMRYHEYDQMRTG4>
.
You are receiving this because you modified the open/close state.Message
ID: ***@***.***>
|
| require('./src/echo'); | ||
| require('./src/error'); | ||
| require('./src/errorCode'); | ||
| // require('./src/exec-child'); excluded since it is for commandline only |
There was a problem hiding this comment.
I think you should change this to just actually require() the exec-child.js file. If you rebase on the lastest master branch, then I have changed this to be a NOOP. I think requiring is necessary to trick the bundler into realizing this file is necessary.
|
I'm going to approve now and land this. I'll send the necessary patches for followup fixes. Thanks for your contribution! |
No change to logic. This is a follow up to PR #1119. This change: * Deletes commands.js entirely and edits the 'files' section of package.json * Undoes some duplicate imports (ex. src/error) * Adds an explicit import for exec-child.js as a hint for the bundler After this change, I expect that bundlers/minifiers such as esbuild will now correctly process shelljs. Fixes #1160
No change to logic. This is a follow up to PR #1119. This change: * Deletes commands.js entirely and edits the 'files' section of package.json * Undoes some duplicate imports (ex. src/error) * Adds an explicit import for exec-child.js as a hint for the bundler After this change, I expect that bundlers/minifiers such as esbuild will now correctly process shelljs. Fixes #1160
|
Ack I am so sorry, I've been busy. Thank you very much for your help and getting this merged in. |
Currently anything that includes shelljs in it's chain cannot be bundled into a singular file due to the dynamic require. By explicitly requiring everything in src, this allows singular bundles through things like esbuild.
No change to logic. This is a follow up to PR shelljs#1119. This change: * Deletes commands.js entirely and edits the 'files' section of package.json * Undoes some duplicate imports (ex. src/error) * Adds an explicit import for exec-child.js as a hint for the bundler After this change, I expect that bundlers/minifiers such as esbuild will now correctly process shelljs. Fixes shelljs#1160
Currently anything that includes shelljs in it's chain cannot be bundled into a singular file due to the dynamic require. By explicitly requiring everything in src, this allows singular bundles through things like esbuild.