Add externalBuiltins option or make them external by default.

[lukastaegert]

This is a proposal in response to https://twitter.com/SavePointSam/status/975146310855622656

At the moment if someone wants to bundle everything but keep builtins as external dependencies, they have to do something like this:

import resolve from 'rollup-plugin-node-resolve';

export default ({
  input: 'src/index.js',
  plugins: [resolve()],
  external: ['path', 'fs', ...],
  output: {
    file: 'build/server.js',
    format: 'cjs'
  }
})

i.e. they have to list all builtins manually in their externals list (and adapt this manually in the future). My feeling is that rollup-plugin-node-resolve already has the necessary dependencies to trivially implement an option externalBuiltins to do that for the user, i.e. the example could become

import resolve from 'rollup-plugin-node-resolve';

export default ({
  input: 'src/index.js',
  plugins: [resolve({externalBuiltins: true})],
  output: {
    file: 'build/server.js',
    format: 'cjs'
  }
})

Would like some feedback before preparing a PR (maybe this is already possible in some way?)
cc @keithamus, @guybedford

[keithamus]

Hm, please correct me if I'm wrong - maybe I'm not understanding the problem - but rollup-plugin-node-resolve doesn't actually resolve builtins. See

rollup-plugin-node-resolve/src/index.js

Lines 172 to 173 in 7846a19

	if ( ~builtins.indexOf( resolved ) ) {
	fulfil( null );

In fact with a small repo using your first config, I get the following output:

$ rollup -c rollup.config.js
src/index.js → build/server.js...
(!) Unresolved dependencies
https://github.com/rollup/rollup/wiki/Troubleshooting#treating-module-as-external-dependency
fs (imported by index.js)
created build.js in 15ms

$ cat build/server.js
'use strict';
var fs = require('fs');
console.log(fs.readFile('x'));

[lukastaegert]

Yes, but it gives you a warning for each builtin that users have to silence by filling the external field. My suggestion is to add a switch to silence these warnings all at once. So the implementation could just become

if ( ~builtins.indexOf( resolved ) ) { 
 	fulfil( externalBuiltins ? false : null ); 

making use of the fact the false is interpreted as "mark as external and do not further try to resolve this via other plugins". But it is also important to not make things more confusing.

[keithamus]

Adding an option just to silence a warning seems - to me - that it does not really meet the complexity-benefit-tradeoff; as you've demonstrated it certainly would be a simple change to implement but another option to document and more complexity inherited by people consuming the plugin. We could instead look to document how to acheive this without changes to this code:

import resolve from 'rollup-plugin-node-resolve';
import builtins from 'builtin-modules'
export default ({
  input: 'src/index.js',
  plugins: [resolve()],
  externals: builtins,
  output: {
    file: 'build/server.js',
    format: 'cjs'
  }
})

[lukastaegert]

Yes, the alternative would be to add this example to the documentation.

[guybedford]

This is definitely useful, great suggestion!

Could also be worth considering if this should be a more generic node: true build option as well.

[mmomtchev]

Anything new on this subject? How do you do it at the moment, you add all the built-ins to external?

[franhaselden]

Also interested in this

[dima-takoy-zz]

Any updates?

[keithamus]

@mmomtchev @franhaselden @mecurc please take a look at #146 (comment); providing builtins to the externals option sufficiently silences these warnings.