Build halts prematurely with no error for incorrectly configured build

[skeggse]

Here's a repro repo. Run npm run broken to see the broken behavior, and npm run working for the behavior when we have the correct config.

We have two modules a and b. a is a commonjs module. a uses functionality exposed by b. a resolves b using some other plugin (i.e. not by a relative require). a is included in the commonjs.include config, but b is not. b can be a commonjs module or an es native module.

Expected behavior: we get an error.

Actual behavior: no error, exit code 0.

This is super distilled from the original problem I had, which also involved rollup-plugin-node-resolve and rollup-plugin-alias, but this seems to be exclusively a problem with the commonjs plugin.

[TrySound]

Had this problem a few times. Really frustrating. Thanks for the repro.

[keithamus]

I've been digging into this a tiny bit. As far as I can tell it falls down to the following:

• load('first') gets called it gets skipped as it doesnt begin with commonjs-proxy
• transform('first') gets called, it gets transformed. All good
• load('second') gets called. Skipped again because commonjs-proxy is missing
• load('commonjs-proxy:second') gets called. Finally we have a hit!
  • this calls isIsCjsPromise (from getIsCjsPromise(id)). This is a new promise and does not resolve to anything yet. This is the important bit.
• transform('rollup-cjs-repro/src/second.js') gets called
  • filter( id ) gets called, and because second isn't in the includes list, it just gets ignored. null is returned.
• meanwhile getIsCjsPromise(id) never resolves. Somehow the process doesn't hang, but load('commonjs-proxy:second') never got resolved.

I believe the potential fix for this is the following:

diff --git src/index.js src/index.js
index 64a3991..16c23ed 100644
--- src/index.js
+++ src/index.js
@@ -205,7 +205,7 @@ export default function commonjs ( options = {} ) {
 		},
 
 		transform ( code, id ) {
-			if ( !filter( id ) ) return null;
+			if ( !filter( id ) && !isCjsPromises[id] ) return null;
 			if ( extensions.indexOf( extname( id ) ) === -1 ) return null;
 
 			const transformPromise = entryModuleIdsPromise.then( (entryModuleIds) => {

The problem with this fix is that it means the includes/excludes options aren't really honoured. Another option is the following patch:

diff --git src/index.js src/index.js
index 64a3991..1ec818d 100644
--- src/index.js
+++ src/index.js
@@ -205,7 +205,10 @@ export default function commonjs ( options = {} ) {
 		},
 
 		transform ( code, id ) {
-			if ( !filter( id ) ) return null;
+			if ( !filter( id ) ) {
+				setIsCjsPromise(id, null)
+				return null;
+			}
 			if ( extensions.indexOf( extname( id ) ) === -1 ) return null;
 
 			const transformPromise = entryModuleIdsPromise.then( (entryModuleIds) => {

The above means the build completes but of course second is treated like an external import, so out.js is missing it and you get a strange error:

(!) Missing exports
https://github.com/rollup/rollup/wiki/Troubleshooting#name-is-not-exported-by-module
commonjs-proxy:/[...]/rollup-cjs-repro/src/second.js
default is not exported by src/second.js

@lukastaegert what are your thoughts about this? I'm happy to make a PR but I'm not sure either of these is ideal.

[lukastaegert]

Please hold off on PRs as I started refactoring a lot of things in #331 and I would not want to merge this yet. Or rather, you could make a PR vs. #331.

Digging through the code myself for the past few days, this is what I know so far myself:

• the isCjsPromise is only set in the transform function. This is the best place IMO as previous transforms such as transpiling JSX will be applied first
• the current logic works by adding imports TWICE in the transform function: The actual import uses the proxy while another import of the non-proxied file is added before this one only to make sure that the isCjsPromise is set correctly. This is what you observed yourself.

Cf.

rollup-plugin-commonjs/src/transform.js

Line 311 in d449220

return `import '${source}';`;

and the comment just before this line

I tried but I could not think of a better solution for this general problem yet.

The big problem about the isCjsPromise situation is that there is no hard connection between the code setting the Promises and the code reading the Promises i.e. there are no guarantees Promises are always resolved. Will think about this a little more tomorrow, would be good if there is a way to establish such a connection. Ideally, requesting a Promise would trigger a transform but I am not sure how this could be done.

[lukastaegert]

Ok, I back-pedalled on #331 as it was just getting too large, so I think I will go for one of the interim solutions above so far.

As a long term solution, I currently think the best would be to implement a new function on the plugin context, something like fetchModule(id): ast which is tied into rollup's internal fetchModule mechanism. It would basically resolve, load and fetch a module the same way rollup would do it and return the resulting ast. Being tied into rollup, it would also make sure that each module is fetched at most once and otherwise return the cached result.

This function could then be called from within the load hook to check the actualid and decide within load

• if the actual id is a CJS file,
• otherwise if the resulting (ESM) AST has a default export (which helps us to optimize the wrapper)

This would help to resolve quite a few potential discrepancies. But I think this is future talk for now.