Proposal: Add support for full ES-Node interop

[ctjlewis]

• Rollup Plugin Name: node-resolve, commonjs, json

• Rollup Plugin Version: latest @ all

Adding support for full ES-Node interop

An important functionality for Rollup I believe is the ability to bundle ES modules which depend on Node packages, as this is a highly common development scenario. Interop with Node typically requires the inclusion of three plugins (above) which I will call the Node compatibility suite, and it presently has 86% coverage over the top ~100 NPM packages and the modules they depend on. Node interop is really the only reason I have ever needed to reach for Webpack anymore, and it would be nice to get us to the finish line on this.

Unfortunately, because a failure at any point in a dependency tree is disastrous to the developer experience, this risk compounds rapidly, and even 99% coverage is insufficient - however, I think it would be relatively easy to get 100% coverage over the top 1000 NPM packages within 30 days, as most issues seem to be caused by a few isolated issues.

Link: Node-Rollup Test Suite [PRs extremely welcome]

There are two types of errors that can occur with Node-Rollup interop:

1. There is a build-time failure, i.e. the output bundle cannot be generated.
2. There is a run-time failure, i.e. the output bundle does not behave in the same way as the source¹.

Some build-time headaches this team is already familiar with include the notable fsevents issue, which I would honestly place as a principal issue, if only because it actually prevents Rollup from bundling its own API via import { rollup } from rollup, which is incredibly depressing. As I understand it, chokidar can only bundle because a chokidar-specific wrapper was added, but this is insufficient for the obvious reason that it does not actually solve the problem - a better option would be adding the ability to ignore optionalDependencies, or even making them opt-in and ignore by default (what I would recommend), which is a future-proof and more general solution that will cover all packages.

---

¹ In the test suite, this is lazily evaluated as "if both fail, pass; if both exit 0, pass" when comparing source and bundle exit codes. Nonzero exits are generalized as "equivalent" for the purposes of these tests. This check can get more aggressive once we have 100% coverage, i.e. making sure console.log is 1:1 and so on.

Expected Behavior / Situation

It should be virtually impossible to find an NPM package which will not bundle and execute as expected.

Actual Behavior / Situation

Of the test sample (n=1303), 17 (1.3%) produce build-time errors, and 176 (13.5%) fail the runtime test, bringing total coverage to ~85.3%. Honestly, I would say this is pretty good coverage going into this issue.

Modification Proposal

Any adjustments could likely be limited to just the node-resolve plugin, and adding the node-resolve plugin to your build would just imply commonjs and json could be enabled with an option flag like nodeResolve({ legacyMode: true }).

Fix build-time failures

1. Add opt-in/opt-out ignore of optionalDependencies to handle fsevents-like scenarios. Preferred solution is to ignore optionalDependencies by default and add a flag for including them.

2. ✅ Fix hashbang directory issue Bundle fails for form require('../#/folder') due to hashbang-only directory #528 (in-progress at fix(babel): strip hash and query param in extension filter #533), which breaks for es5-ext package (and packages in the top 100 that depend on it, specifically d gulp es6-iterator es6-symbol es6-weak-map gulp gulp-cli last-run semver-greatest-satisfied-range sver-compat, see config/exclude.txt). [Update 11/13/20: This is fixed (presumably) with fix(babel): strip hash and query param in extension filter #533.]

3. Fix other build-time errors produced by the packages in config/exclude.txt. There's a "return outside of function" in there and a few other issues, but I have not bothered logging the build-time errors in the results/ dir yet as the runtime errors are much harder to test and outnumber them 10:1.

Packages which cannot build are added to config/exclude.txt so that the tests will build, any package in the test sample which refuses to build is listed in there.

Fix run-time failures

The following two issues cause 50% of the run-time errors:

1. util.inherits transpilations will fail in certain cases and get called with a null second argument (admittedly need help debugging this one), causing the error TypeError [ERR_INVALID_ARG_TYPE]: The "superCtor" argument must be of type function. Received undefined to be one of the two most common runtime errors, occurring for 64 packages that were tested for runtime errors (36% of build-time failures). See bundles/argparse.js:983: util.inherits(ArgumentGroup, action_container). This error seems most frequently produced by packages that rely on readable-stream.

2. __filename and __dirname shims are needed as CJS Node modules make use of these. I considered the node-polyfills plugin, but it completely conflicts with node-resolve and makes the build process unworkable. Errors due to __filename and __dirname being rolled into ES output cause 25 packages to fail (14% of build-time failures). This support will need to be added in a more native manner, see [commonjs] Output contains references to __filename #523.

---

I am happy to help as much as I can with this process, but I am a little out of my depth here and I would really appreciate support from the Rollup team in reaching 100% Node coverage.

[ctjlewis]

@Andarist @lukastaegert Thoughts?

[shellscape]

and adding the node-resolve plugin to your build would just imply commonjs and json.

I've got several builds where this would fail hard. Can't go into details, but I'm certainly not in favor of adding magic here. Config should be explicit imho.

Beyond that, we'd welcome any help from contributors on proposing these changes. Much of this seems reasonable though I think we're all fairly short on time at the moment.

[LarsDenBakker]

I agree that node-resolve should not imply commonjs and json, they all mean completely different things. If you're building a browser or node es module project, you want to use node resolve but not the others.

I can imagine some kind of environment "preset" would be nice. Or at least some good documentation on this. For example "node commonjs", "node es modules", "browser es module" etc. which would combine different plugins as well as set some default properties. That would be very helpful for people who don't (want to) know all the technical nuances.

If I understand correctly, your suggestion is to treat optionalDependencies as externals by default? I'm not sure if that's appropriate to do by rollup, I don't think it reads the user's package.json right now.

The other suggestions seem reasonable, but like @shellscape they can be tackled with individual issues and PRs :)

[ctjlewis]

The reason I said node-resolve should imply commonjs and json is because if we want to bundle arbitrary Node dependencies, we will likely end up encountering CJS modules that end up importing JSON, somewhere in the dependency tree.

I've got several builds where this would fail hard.

That is unremarkable - Rollup cannot even successfully bundle itself with this stack and execute correctly (womp womp) due to the fsevents nightmare that plagues this and Webpack alike. It will fail hard about 14% of the time. My entire point is that it is not really possible to bundle arbitrary Node packages even when these plugins are specified explicitly.

I suppose a more realistic and tapered version of what I am proposing is getting Node compatibility to 100% coverage and then adding a new plugin like node-compatibility or something (though that doesn't really make sense to me, as this is basically just node-resolve except it works for old CJS modules). The goal is to just let a person do stuff like import chalk; console.log(chalk.blue('hello'));, bundle it down to builtin imports only (resolving all ES/CJS/JSON imports in a backwards compatible way), and produce output that executes correctly.

I don't think it reads the user's package.json right now.

Me neither - my point is that the only way to get out of fsevents hell is to read optionalDeps for each Node import and mark them external unless opted in (as I understand it, currently only chokidar will bundle and execute with an fsevents dep, since a custom wrapper was added for chokidar specifically). The only way I was able to get around this was by explicitly writing a plugin that just removes fsevents import statements from output bundles completely, that's how messy this is.

After wrestling with bundles that just keep containing fsevents imports, it seemed like the obvious solution to this would be to mark optional deps as external unless opted in, as by definition they won't break if not included, and if they are installed as a peerDep or whatever they will be available - this could be done with a plugin I suppose, but it is really worth considering for the stability of the core tool IMO. At the very least, we should evaluate how to handle of fsevents-style, edge case optionalDeps in a more proactive way.

I'm not sure if that's appropriate to do by rollup.

It is perfectly appropriate for Rollup to assess the dependencies of a module it is bundling, and make decisions based on that information.

My main goal in accomplishing this was to produce output that I can run through Closure Compiler, but I ended up having to build an entire tool to get it to work. You can see the plugin stack at: https://github.com/TeleworkInc/.gnv/blob/master/rollup.plugins.js

export const distPlugins = [
  shebang(),
  /**
   * Input is Closure Compiled to minimize strain on `node-resolve`.
   */
  closureCompiler({
    define: 'PRODUCTION=true',
    compilation_level: 'SIMPLE',
    language_in: 'ES_NEXT',
    language_out: 'NO_TRANSPILE',
  }),
  commonjs({
    transformMixedEsModules: true,
  }),
  json(),
  disablePackages('fsevents'),
  nodeResolve({
    preferBuiltins: true,
  }),
  bundleSize(),
];

With externals at: https://github.com/TeleworkInc/.gnv/blob/master/rollup.externals.js

export const distExternal = [
  /**
   * Builtins and manually disabled packages.
   */
  ...builtinModules,
  ...disabledModules,
  /**
   * Package.json dependencies.
   */
  ...peerDeps,
];

[LarsDenBakker]

Rollup core doesn't have any concept of packages or dependencies, it just bundles es modules which refer to each other relative or absolute paths. Everything else is brought in via plugins.

The node-resolve plugin only handles resolving bare imports using the node resolution algorithm. A feature initiated by node js, but now used in other environments as well including the browser. For the browser you will use this plugin, but not the others. Similarly some people building native node es module may only want to bring in json module support.

Having these separate plugins is important in my opinion. Especially since bundling non-es modules is inherently broken, what works for one package will break another package. Still, having some kind of preset or multi-plugin would be nice to have especially for legacy node js.

[danielgindi]

I think that a "preset" would be nice. Plugins should not imply each other - as in some situations they could make a lot of mess when unintended - and the dev will have a hard time disabling "implied" stuff.

In my opinion a preset is definitely the way to go. But that's a concept yet to be introduced to rollup.

Another note - one of the things I love about rollup is that it encourages transitioning to static imports and teaches its benefits. It encourages proper code structure without circular dependencies that depend on quirks in cjs require algorithm.
There's no point in doing "webpack" and just pack everything in one big messy package, as webpack already exists. Rollup works more like a compiler, optimizing stuff instead of bloating stuff.

[ctjlewis]

The amount of focus on the "implied plugin" comment is a little confusing - the bulk of this proposal is not about implying the commonjs and json plugins by default (and basically turning it into a type of preset as you mentioned), but rather about how even if you do include all of the necessary plugins in a "legacy stack" (which I included an example of), the output bundles for various legacy Node modules in the wild will fail at run-time (and build-time) due to various errors. I did say I think that should happen by default (I would expect node-resolve to fail 0% of the time), but that is not the main focus as much as the ability to do so (even if opt-in).

Put shortly, there are a lot of Node modules in the wild Rollup cannot compile with standard plugins. My suggestion is to fix this.

It is very clear based on the responses that we do not want to change default functionality (for node-resolve or, I imagine, anything), and I am completely understanding of that - my goal is just to be able to bundle a package that depends on one of several hundred top NPM packages and have the output work as expected. I have accomplished this for my own workspaces by adding an entire custom tooling layer, but it is not possible out of the box with Rollup and standard plugins.

My apologies if I was not clear in my original phrasing but genuinely, please close this issue rather than leave additional comments regarding my comments about "implied" plugins. I tried to do my part and bring an interesting issue forward and am willing to help work to address it if anyone's interested, but if not, please just close the issue.

[lukastaegert]

I think the ticket is important but it contains a lot of points that might be better served by splitting them up into separate tickets as many of them will likely be addressed independently anyway and require independent discussion. The handling of optionalDependencies for instance is entirely within what the node-resolve plugin can do as it DOES read package files, BUT this will require a logic for custom package-based resolution. I.e. what is optional for one package may not be optional for another package. This might be coupled therefore with a solution for handling the new package.imports which is also concerned with package-based resolution.

The second point is apparently already in progress while the third point sounds like a collection of issues, each of which needs to be identified and triaged separately.

[fregante]

Presets are already kind of possible via --config node:foo to import rollup-config-foo. The problem is that as far as I know it's not possible to extend them with multiple --config flags.

If this was possible, it'd be easier to reach browserify-levels of node compatibility:

rollup --config node:node-compat --config rollup.config.js

[aral]

@ctjlewis Thank you for this proposal.

I’m looking into whether to implement SvelteKit in Site.js and I’m being blocked by having issues with, for example, modules like eff-diceware-passphrase (@^1.0.0 as @^3.0.0 has issues with browserify also) which work off the bat with browserify (and Snowpack) but which I haven’t been able to get to work yet using Rollup even when using the three plugins you mention.

Having a plugin like node-compatibility, as you suggest, that just handles ES-Node interop would be a game changer.

[ctjlewis]

@aral Agreed. I ended up having to develop a workspace preset and series of configs to work around these issues, and while this is totally not ready for public use, you can take a look at https://github.com/TeleworkInc/gnv.

I would be happy to work on this more if the Rollup team were interested. This test suite was super lazily written and could be redone with a more careful touch, but I'd need help, and it would need to be prioritized.

Confusingly, @Rich-Harris says that Snowpack relies on Rollup under the hood, so I wonder how interop is handled in that library?

[shellscape]

@FredKSchott is super nice and might be able to shed light there

[FredKSchott]

Check out https://www.npmjs.com/package/esinstall, it’s Snowpacks internal package CJS->ESM installer, powered internally by rollup.

[aral]

it’s Snowpacks internal package CJS->ESM installer, powered internally by rollup.

Neat; thank you :)

[shellscape]

Ran across this today https://github.com/niksy/node-stdlib-browser#rollup