Add ability to include node built-in modules as external dependencies

[krismuniz]

Hello 👋

This PR adds the ability to include node built-in modules as external dependencies by specifying --external natives

Related to #288, but I'm not 100% sure if it closes it, as the PR suggests using the rollup-plugin-node-builtins plugin for resolving these dependencies and including shims where necessary (i.e. in the browser).

Problem

When using microbundle for building node modules (--target node), you have to include every built-in module like this:

microbundle --external events,util,child_process

This becomes even more tedious when a built-in module uses other built-in modules.

As @lukeed suggests in this comment, it would be nice to have the option to simply treat all built-in node modules as external dependencies by adding --external natives instead of --external events,util,child_process[, ...].

Solution

Add all modules from require('module').builtinModules to the list of external dependencies like this:

microbundle --external natives

Feedback Welcome

✏️Let me know if you need any changes
🗯If this is not ideal, what would be a nice alternative to specifying all built-in modules?

[lukeed]

🔥 You're killin' it @krismuniz~!

I wouldn't rush to change anything (others should weigh in first), but I think this should be on by default, replacing the minuscule natives-exception list (here).

Right now, using --external natives will prevent you from also making your (peer)dependencies external too, unless you declared them all individually, too.

If we have all natives on by default, a local dependency with the same name should/will already be external & should/will still be found when required.

[krismuniz]

I think this should be on by default, replacing the minuscule natives-exception list (here).

@lukeed that would be even better! Something like this, right?

- let external = ['dns', 'fs', 'path', 'url'].concat(/* ... */);
+ let external = require('module').builtinModules.concat(/* ... */);

---

Right now, using --external natives will prevent you from also making your (peer)dependencies external too, unless you declared them all individually, too.

Indeed, if for some reason it were impractical to just include them by default we could also make natives equal all built-in node modules like this:

external = external.concat(peerDeps).concat(options.external.split(','));
// if options.external contains 'natives', include built-in modules
external = options.external.split(',').includes('natives')
  ? external.concat(require('module').builtinModules)
  : external;

---

If we have all natives on by default, a local dependency with the same name should/will already be external & should/will still be found when required.

💯yes, that would be the ideal scenario, imho

[lukeed]

Something like this, right?

Correct!

The only other thing to consider here is that require('module').builtinModules doesn't work in Node 4.x – hence my two-part suggestion in the original issue. I don't know/can't remember the minimum supported Node version microbundle is targeting.

[Andarist]

I also agree that this should be the default with node as target, we could use this static list for this - https://github.com/sindresorhus/builtin-modules/blob/master/builtin-modules.json

[lukeed]

Don't think we need an extra dependency – the list is built-in to Node in all versions:

require('module').builtinModules || Object.keys(process.binding('natives'))

[krismuniz]

Right! So if we want Node 4.x support we'd do something like this:

- let external = require('module').builtinModules.concat(/* ... */);
+ const builtinModules = require('module').builtinModules || Object.keys(process.binding('natives'));
/* ... */
+ builtinModules.concat(/* ... */);

Would this apply only when target === 'node'?

[developit]

@krismuniz it should only apply to target==='node' yup.

If we want to support both automatic builtin externals for target=node and --external natives, perhaps the following?

if (target=='node' && external.indexOf('natives')===-1) {
  external.push('natives');
}
// ...
const i = external.indexOf('natives');
if (i!==-1) {
  const builtins = require('module').builtinModules || Object.keys(process.binding('natives'));
  external.splice(i, 1, builtins);
}

[ianstormtaylor]

Any news on this? This would be super helpful.

[ForsakenHarmony]

Problems mentioned above would have to be solved

[developit]

FWIW this behavior is already enabled by default in microbundle@next, since rollup-plugin-node-resolve handles it by default. I've opened #591 to silence the warnings.

Note: we could still merge this PR, though I'm not sure it does anything as of the update.