Explicitly require the commands to help JS bundlers find all the code

[lambrospetrou]

Node version (or tell us if you're using electron or some other framework):

v12.8.1

ShellJS version (the most recent version/Github branch you see the bug on):

0.8.3

Operating system:

Ubuntu 18.04.3 LTS

Description of the bug:

First of all, thanks for an amazing library, it has been working great for me in all cases I needed it. Apart from today that I tried bundling one of my Node apps.

It would be great if the calls to requires were explicit and not dynamic as it's done at

shelljs/shell.js

Lines 24 to 26 in 57df38c

	require('./commands').forEach(function (command) {
	require('./src/' + command);
	});

Reason is that when we want to bundle the whole Node application/script into a single JS file, it's impossible to get it to bundle correctly.

One example is something I was trying to do today using Shadow-CLJS; more details at thheller/shadow-cljs#290 (comment)

I tried almost every popular bundler, including @zeit/ncc, Parcel-bundler, Rollup, and finally Webpack.

To be honest, I believe webpack managed to resolve the dynamic requires but then had other issues with __dirname which I use in my code (whole other different issue).

However, it would be much easier to integrate with these bundlers if the requires calls were explicit. Any specific reason why you go through the indirection of array of strings and a loop to require versus the direct require of the commands (other than saving a few characters)?

Thanks

[nfischer]

Even if we did as you've suggested, bundlers probably would not recognize src/exec-child.js as a dependency of src/exec.js, which would break our module. Our module expects every file we use to be available on the system, and we express this dependency explicitly in package.json's files property.

If your bundlers are ignoring the npm-supported ways of expressing which files are necessary, then these bundlers are likely to break the npm ecosystem.

If these bundlers support APIs to tell them which files are necessary, I'm happy to take a look if these make sense to call in ShellJS, or in dependent modules.

[lambrospetrou]

Even if we did as you've suggested, bundlers probably would not recognize src/exec-child.js as a dependency of src/exec.js, which would break our module.

You are right. It seems that webpack manages to bundle the commands, but not the exec-child.js.

I used the following configuration in webpack.config.js:

const path = require('path');
module.exports = {
  target: 'node',
  node: {
    __dirname: false
  },
  mode: 'development',
  entry: './src/js/index.js',
  output: {
    filename: 'lib.js',
    path: path.resolve(__dirname, 'dist'),
    // library: 'dummyLibName',
    // libraryTarget: 'umd'
  }
};

and with the following source code in src/js/index.js:

const sh = require("shelljs");
console.log(":: SHELLS JS");
sh.echo(`exec: ${sh.exec("ls")}`);
console.log(":: SHELLS JS END ::");

and running the output file in a directory without the node_modules folder throws the following error which indeed proves that exec-child.js is not part of the bundle.

lambros@thinkunix:~/dev/tmp/app1$ node ~/dev/github/create-shadow-cljs-app/dist/lib.js
:: SHELLS JS
internal/modules/cjs/loader.js:716
    throw err;
    ^

Error: Cannot find module '/home/lambros/dev/github/create-shadow-cljs-app/dist/exec-child.js'
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:713:15)
    at Function.Module._load (internal/modules/cjs/loader.js:618:27)
    at Function.Module.runMain (internal/modules/cjs/loader.js:931:10)
    at internal/main/run_main_module.js:17:11 {
  code: 'MODULE_NOT_FOUND',
  requireStack: []
}
webpack:///./node_modules/shelljs/src/common.js?:399
        throw e;
        ^

Error [ShellJSInternalError]: ENOENT: no such file or directory, open '/tmp/shelljs_26a91aa1f2f4ad713060'
    at Object.openSync (fs.js:447:3)
    at Object.readFileSync (fs.js:349:35)
    at execSync (webpack:///./node_modules/shelljs/src/exec.js?:89:17)
    at Object._exec (webpack:///./node_modules/shelljs/src/exec.js?:205:12)
    at Object.eval [as exec] (webpack:///./node_modules/shelljs/src/common.js?:335:23)
    at eval (webpack:///./src/js/index.js?:5:21)
    at Object../src/js/index.js (/home/lambros/dev/github/create-shadow-cljs-app/dist/lib.js:660:1)
    at __webpack_require__ (/home/lambros/dev/github/create-shadow-cljs-app/dist/lib.js:20:30)
    at /home/lambros/dev/github/create-shadow-cljs-app/dist/lib.js:84:18
    at Object.<anonymous> (/home/lambros/dev/github/create-shadow-cljs-app/dist/lib.js:87:10) {
  errno: -2,
  syscall: 'open',
  code: 'ENOENT',
  path: '/tmp/shelljs_26a91aa1f2f4ad713060',
  name: 'ShellJSInternalError'
}

[nfischer]

Is there an API to tell webpack to keep this file?

[lambrospetrou]

Hmm, not sure if there is a generic way to do it through package.json to be honest so that other bundlers could hook on it as well (now or in the future).

Webpack has a few tricks around this but they are specific to it. I will need to play around a bit more and try a few things this week to see if I can get something working, and then we can see if it's something that can be merged into the main package.

[lambrospetrou]

I just realized that exec-child.js is supposed to be an actual file since exec.js uses execFileSync passing the path to exec-child.js.

Since webpack and other bundlers follow through the requires/import directives to find all dependencies this is not bundled at all, and even if it was (with some magic), since the output of what I want to do is just to have a single JS file containing everything it would again fail when executing the execFileSync command.

I am not too familiar with Node's execXXX family but is there any way to get the same functionality without needing an actual file on disk that will be executed by node? For example, could we wrap the code inside exec-child inside a function and then call that function instead of passing a path to the file to be executed?

Might be a very stupid question but you probably already went through most of Node's API on this so might worth a shot asking anyway;p

Thanks though in any way.

[nfischer]

The path forward would be to deprecate and later remove shell.exec(). I scoped out a potential replacement (shell.cmd(), #495), but even this would probably have an off-by-default "real-time stdio" mode, which would then require shelling out to a file on disk.

If bundler-users are OK with missing out on real-time stdio (instead, you would get stdout/stderr only as a pair of javascript strings) then this is probably fine.

[wenerme]

How about pre generate imports when build like this vercel/ncc#476 (comment) still keep the commands file, but help bundler

[nfischer]

ShellJS expects our own files will be present at runtime. If your bundler does not meet this expectation, we consider this a bug of the bundler. We will not generate require statements for the sake of tricking bundlers into keeping our files, especially in the case of shelljs/src/exec-child.js, which is not meant to ever be required.

Your bundler should examine our package.json's files attribute and include all the files there, as this is what we've specified our module requires in order to run correctly.

How about pre generate imports when build like this zeit/ncc#476 (comment) still keep the commands file, but help bundler

I'm not sure what you mean. Are you proposing changes to ShellJS or to the bundler?

[wenerme]

@nfischer for pre generate I mean something like
package.json

{
  "scripts":{
    "build":"node scripts/generate-command-imports.js && <original>"
  }
}

scripts/generate-command-imports.js

import commands.js write real imports to the file

But you think the bundler should handle this, this tricks can also used as a workaround by those who use the bundler that can not handle this.

[kokokenada]

ShellJS expects our own files will be present at runtime. If your bundler does not meet this expectation, we consider this a bug of the bundler

The purpose of rollup etc, is to package the sources into a distribution. So, the design choice of requiring files to be present makes shelljs incompatible with bundlers. I was trying to use shelljs in a cli and wanted to use rollup in order to simplify the distribution. Is there a major downside to using explicit requires?

[shakedlokits]

Hey @nfischer, can you please help with this?
What do I configure typescript with?
I'm getting this error, it's clearly because of how typescript bundles, but how do I configure it?

[7PH]

For anyone affected, I've opened a new issue suggesting a fix for shelljs, and a workaround for esbuild using a plugin esbuild-plugin-shelljs. Hopefully this can help you.