Rollup does not preserve order of imports after commonjs

[danielgindi]

• Rollup Version: 2.28.2
• Operating System (or Browser): MacOS, repl.it
• Node Version (if applicable): v12.19.0
• Link to reproduction (IMPORTANT, read below): https://repl.it/repls/PricklyBuzzingCode#package.json

Expected Behavior

All commonjsRegister calls should be at the top.
The commonjs plugin generates them at the top, and it should stay that way.

In cases of import from 'whatever', the import is clearly meant for causing a side effect, and thus it's order of execution matters.
(Of course it could be also in normal imports, but this case is very explicit and well known).

Actual Behavior

The order of imports generated by the commonjs plugin is not preserved, and other modules appear in between.
The result in some cases is that there's a call to commonjsRequire before the dynamic modules have been registered, so they it can't find/generate the module during the commonjsRequire call.

In this specific REPL you can see that between the commonjsRegister(...internal/wrapAsync) and commonjsRegister(...asyncify), there's one piece of code var setImmediate_1 = createCommonjsModule(...), which is extra weird since this variable is not in use yet, so why bump it to that position on top?

I know that rollup tends to manage the order of things by itself for the sake of optimization,
but we need to think of a way to at least specify cases where the user (or a plugin) requires a steady order of imports.
And perhaps detect those "side effect" imports and handle those automatically.

[lukastaegert]

I do not think Rollup is messing up here but the commonjs plugin, but one would need to look deeper. There are situations where Rollup can mess up import order, but this is only possible in the presence of chunks or external dependencies, neither of which seems to be the case here.

The first step would be to log all modules as they are passed to Rollup, e.g. by adding a plugin

{
  transform(code, id) {
    console.log('===>', id); 
    console.log(code);
  }
}

and the noting all imports to plot the canonical execution order. Then it should become clear what is going wrong here.

[lukastaegert]

The reason is indeed simple: asyncify.js is importing setImmediate.js, which means that the latter is executed first. I created a script that plots the correct execution order including how it comes to pass by using indentation to denote dependencies. input.js indeed has four dependencies after the commonjs plugin: internal/wrapAsync.js, asyncify.js, series.js and series.js?commonjs-proxy. This is what I get:

(imported by input.js):
-(imported by internal/wrapAsync.js):
--commonjsHelpers.js
--commonjsHelpers.js?commonjsRegister
-internal/wrapAsync.js
-(imported by asyncify.js):
--internal/initialParams.js
--internal/setImmediate.js
--internal/initialParams.js?commonjs-proxy
--internal/setImmediate.js?commonjs-proxy
-asyncify.js
-(imported by series.js):
--(imported by internal/parallel.js):
---internal/isArrayLike.js
---internal/awaitify.js
---internal/isArrayLike.js?commonjs-proxy
---internal/awaitify.js?commonjs-proxy
--internal/parallel.js
--(imported by eachOfSeries.js):
---(imported by eachOfLimit.js):
----(imported by internal/eachOfLimit.js):
-----internal/once.js
-----(imported by internal/iterator.js):
------internal/getIterator.js
------internal/getIterator.js?commonjs-proxy
-----internal/iterator.js
-----internal/onlyOnce.js
-----(imported by internal/asyncEachOfLimit.js):
------internal/breakLoop.js
------internal/breakLoop.js?commonjs-proxy
-----internal/asyncEachOfLimit.js
-----internal/once.js?commonjs-proxy
-----internal/iterator.js?commonjs-proxy
-----internal/onlyOnce.js?commonjs-proxy
-----internal/asyncEachOfLimit.js?commonjs-proxy
----internal/eachOfLimit.js
----internal/eachOfLimit.js?commonjs-proxy
---eachOfLimit.js
---eachOfLimit.js?commonjs-proxy
--eachOfSeries.js
--internal/parallel.js?commonjs-proxy
--eachOfSeries.js?commonjs-proxy
-series.js
-series.js?commonjs-proxy
input.js

[lukastaegert]

So the correct execution order was indeed

... -> wrapAsync.js -> initialParams.js -> setImmediate.js -> initialParams.js?commonjs-proxy -> setImmediate.js?commonjs-proxy -> asyncify.js

which is what Rollup did. This is indeed more of a fundamental issue with the commonjs plugin here because it does not account for the fact that dynamic modules can have dependencies themselves. Not sure if there is an easy fix.

[danielgindi]

So basically when it commonjs parses this:

const commonjsRegister = require('\0commonjsHelpers.js?commonjsRegister');
commonjsRegister("/$$rollup_base$$/node_modules/async/asyncify.js", function (module, exports) {
  'use strict';

Object.defineProperty(exports, "__esModule", {
    value: true
});
exports.default = asyncify;

var _initialParams = require('./internal/initialParams');    // <<----
....

It translates it into this:

import * as commonjsHelpers from '\0commonjsHelpers.js';
import '\0commonjsHelpers.js?commonjsRegister';
import './internal/initialParams';
import './internal/setImmediate';
import commonjsRegister from '\0commonjsHelpers.js?commonjsRegister';
import require$$0 from '\0./internal/initialParams?commonjs-proxy';
import require$$1 from '\0./internal/setImmediate?commonjs-proxy';

var asyncify = commonjsHelpers.createCommonjsModule(function (module) {
commonjsRegister("/$$rollup_base$$/node_modules/async/asyncify.js", function (module, exports) {
  'use strict';

Object.defineProperty(exports, "__esModule", {
    value: true
});
exports.default = asyncify;

var _initialParams = require$$0;

var _initialParams2 = _interopRequireDefault(_initialParams);

So as far as rollup is concerned, there are multiple import statements between this commonjsRegister file and the next right?

Now I'm hitting my head against the wall, trying to figure out a way around this.
Maybe populating an array with all of those imports, putting a virtual import after all commonjsRegisters, and unpacking those imports there?

[lukastaegert]

Not sure. So as I understand is, there are several approaches:

• forbid dynamic require targets from having dependencies. This is certainly not what users want but would fix the issue.
• make all dependencies dynamic require targets themselves so that they are only loaded once used. This could actually work but could also have a drastic effect in case one of the targets imports a huge library.
• do some graph analysis to find out what the first module in the execution order is. Add the commonjs register THERE. Problem: This could potentially mess up dependencies and may not work at all if there are several entry points.
• ...

Nothing ideal yet I guess.

[lukastaegert]

@danielgindi I have thought some more about this and I think I know a way forward. So as I understand it what the dynamicRequireTargets feature wants to achieve is two things:

• It should be possible to resolve require calls with arbitrary targets at runtime
• the required files should only be executed once required

I am not 100% sure about the second but if you completely forego the notion that you want to couple this feature with conditional code execution, then it can be made much simpler and remove a lot of the current cognitive complexity out of the commonjs plugin.

The problem is as we saw that conditional execution does not work anyway as transitive dependencies will still be executed on startup, messing up the registration code. This is something to tackle separately IMO, and I would have some vague ideas where to start. (By the way, I just stumbled about the issues when bundling logform myself, which is the example you put into the docs. My fix was to use undocumented deep imports, but I guess a direct solution would be better).

So assume you have

commonjs({
  dynamicRequireTargets: [
    'foo', 'bar'
  ],
});

Then we add the following virtual module:

// \0commonjs-dynamic-packages
module.exports = {
  // this is how you do object literals without prototype
  __proto__: null,
  foo: () => require('foo'),
  bar: () => require('bar')
}

So at the moment, this would be translated to immediate synchronous require calls to 'foo' and 'bar' despite me putting them into functions. But if we implement general conditional require support in this plugin in the future, this feature will immediately benefit from this. Note that this should be a CommonJS file so that we do not get any unwanted interop code.

Then we can change the dynamic commonjs helpers like this:

import DYNAMIC_REQUIRE_CACHE from '\0commonjs-dynamic-packages';

// get rid of DYNAMIC_REQUIRE_CACHE and DYNAMIC_REQUIRE_LOADERS below
// ...

export function commonjsRequire (path, originalModuleDir) {
	const shouldTryNodeModules = isPossibleNodeModulesPath(path);
	path = normalize(path);
	let relPath;
	while (true) {
		if (!shouldTryNodeModules) {
			relPath = originalModuleDir ? normalize(originalModuleDir + '/' + path) : path;
		} else if (originalModuleDir) {
			relPath = normalize(originalModuleDir + '/node_modules/' + path);
		} else {
			relPath = normalize(join('node_modules', path));
		}
		for (let extensionIndex = 0; extensionIndex < CHECKED_EXTENSIONS.length; extensionIndex++) {
			const resolvedPath = relPath + CHECKED_EXTENSIONS[extensionIndex];
			let loader = DYNAMIC_REQUIRE_CACHE[resolvedPath];
			if (loader) return loader().exports;
		}
		if (!shouldTryNodeModules) break;
		const nextDir = normalize(originalModuleDir + '/..');
		if (nextDir === originalModuleDir) break;
		originalModuleDir = nextDir;
	}
	return require(path);
}

and that's all. We do not need to cache the exports of each module because that is taken care of elsewhere.

[danielgindi]

• It should be possible to resolve require calls with arbitrary targets at runtime

• the required files should only be executed once required

The second one is actually a side effect.
The second main point is that circular dependencies should be possible and should act in the exact same way as nodejs does it.
So currently working weird circular-dependency modules- will keep working.

So the cache is part of what allows that. There's a small algorithm in there that enables it in general- by first putting the module.exports in the cache, then running the module so it its circular deps will be able to access its export before the module's first run is done.
Currently if a module sets module.exports to a different value, then a circular dependency will have the wrong export, and this is how it would work here too.

The fact that modules will only be executed once is a sideeffect here, and is not 100%. It actually works like in node, where one could actually access the cache, remove a module from cache and cause it to reload!

So a commonjs module that takes advantage of nodejs module system to the fullest extent- will work the same here.

(By the way, I just stumbled about the issues when bundling logform myself, which is the example you put into the docs. My fix was to use undocumented deep imports, but I guess a direct solution would be better).

Yeah it's one of the most annoying modules in the npm ecosystem. I opened an issue over there but got ignored.
At the time O did the same with knex- and it got resolved right away, so one would only have to add their specific connector as "dynamic", and not the whole knex module as circular dependencies got resolved.

If I understand you correctly, you are suggesting to create a single map at the top, so commonjsRegister calls don't get separated by other imports, as it all happens first.

That's a nice idea, except commonjs translate normal require calls into imports -> imports must be at the top of the file -> rollup will probably move them to the top?

I also had a more complex idea on how to solve this. Basically making any import created inside the commonjsRegister proxy a unique variable name, extracting them, and adding them back after the register calls.

But now I think a faster route there would be a variation on what you suggested!
Add "placeholders" for all the dynamic requires, directly inside the cache at the beginning.
So anyone immediately requiring them would get the empty module.exports, and a piece of code after the last commonjsRegister - will cause all of those that were already accessed to be executed.

---

As a side note- in general the fact that an import got inserted between commonjsRegister should not cripple the execution flow. But I had a situation with that damned async lib that did do just that, and I'm going to dive even deeper now, to try to put my finger on the exact situation and make an exact and simple repro.
I don't want to sweat on fixing something that does not need fixing.
I have a suspicion that there was another "circular dependency" that rollup did not warn about, something else that should definitely be declare as a dynamic target, without adding random modules as dynamic just to workaround the issue.

[danielgindi]

@lukastaegert
As I suddenly suspected, my configuration was flawed.
The issue was with the same package (async) as in the repro, and I did add both parties of the circular dependency to dynamicRequireTargets, but for one of them the subfolder was wrong.
async has a weird dist package. Some is in "dist" folder, some in root folder, some in "internal" folder.
Now they do have a module entry pointing to an .mjs file in the dist folder- but they also encourage users to access specific features directly like require('async/series'), and if you do that instead of a named import - then you go into a vicious circular dependency in the cjs version, and code duplication as other libraries might actually use the .mjs version.

Anyway, it suddenly hit me that the fact the imports are sneaking in between should not be an issue as if they caused an issue then it hints about a circular dependency...

So I deem this a non-issue, a false alarm, and I'm closing this.

---

Just for the sake of developers looking for solutions about specific modules with circular dependencies, I'll list a few I know about here:

• logform: dynamicRequireTargets: ['node_modules/logform{/*.js}']
• glob: dynamicRequireTargets: ['node_modules/glob/{sync,glob}.js']
• inversify: dynamicRequireTargets: ['node_modules/inversify/lib/syntax/binding_{on,when}_syntax.js']
• async: dynamicRequireTargets: ['node_modules/glob/{,internal/}*.js'] - you might want to be specific if warned only about a few pieces of this library, but take care not to miss part of the circular dependency. The "internal" folder is confusing.
• readable-stream: dynamicRequireTargets: ['**/node_modules/readable-stream/**/*.js'] - this one is tricky as there are usually multiple instances of it with different versions. BUT the latest version (not yet released) already follows my advice and removed the circular dependency. So in the near future this won't be necessary, and different versions could be forced to use the latest (with alias plugin).
  Personally when building for node- I just replace these with the internal 'stream' module (using replace modules with empty delimiters)
• winston - either add 'node_modules/winston-transport/*.js', or do this:

    require('@rollup/plugin-replace')({
        'delimiters': ['', ''],
        'require(\'readable-stream\')': 'require(\'stream\')',
        'require("readable-stream")': 'require("stream")',
        'require(\'readable-stream/writable\')': 'require(\'stream\').Writable',
        'require("readable-stream/writable")': 'require("stream").Writable',
        'require(\'readable-stream/readable\')': 'require(\'stream\').Readable',
        'require("readable-stream/readable")': 'require("stream").Readable',
        'LegacyTransportStream = require(\'./legacy\')': 'LegacyTransportStream = null',
        'LegacyTransportStream = require(\'winston-transport/legacy\')': 'LegacyTransportStream = null'
    }),
  

• knex - only add the files related to the specific dialect you are using. the main module files no longer have any circular dependency, but the dialect is dynamically required.

[lukastaegert]

We are finalizing a new version of the commonjs plugin in rollup/plugins#1038. It is pre-published as @rollup/plugin-commonjs@beta. Please give it a spin to see if it resolves the issue, ideally without additional config needed.