Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[@rollup/plugin-commonjs] Circular dependency is not resolved optimally by plugin-commonjs for protobufjs causing runtime error #988

Closed
kskalski opened this issue Sep 2, 2021 · 32 comments · Fixed by #1038

Comments

@kskalski
Copy link

kskalski commented Sep 2, 2021

  • Rollup Plugin Name: plugin-commonjs
  • Rollup Plugin Version: 20.0.0
  • Rollup Version: 2.56.3
  • Operating System (or Browser): Windows / Chrome
  • Node Version: v12.20.1
  • Link to reproduction: https://github.com/kskalski/rollup-protobufjs

Expected Behavior

Code runs without error, in particular code from protobufjs object.js is executed before its reference in field.js thus avoiding use of uninitialized variable ReflectionObject

Circular dependency reported during build, but resolved in favor of executing required (unconditionally specified at root of the module) dependencies first before "loose" dependencies (requires placed in function bodies / conditionals?)

Actual Behavior

Error during runtime

field.js:6 Uncaught TypeError: Cannot read property 'prototype' of undefined
   at field.js:6
   at index.umd.js:4
   at index.umd.js:5

The order of circular dependency resolution is not optimal, since modules that are not on critical path are executed before those on critical path.

Additional Information

In particular I tracked the offending cycle as node_modules\protobufjs\src\object.js -> node_modules\protobufjs\src\util.js -> node_modules\protobufjs\src\type.js -> node_modules\protobufjs\src\namespace.js -> node_modules\protobufjs\src\object.js

however:

  • object.js has a hard dependency at top var util = require("./util");,
  • namespace.js has hard dedepdency at top var ReflectionObject = require("./object");
  • while util.js has only loose dependencies on type.js (and also enum.js and root.js), like those:
util.decorateType = function decorateType(ctor, typeName) {
...
    if (!Type)
        Type = require("./type");
...
}
...
Object.defineProperty(util, "decorateRoot", {
    get: function() {
        return roots["decorated"] || (roots["decorated"] = new Root());
    }
});

so in fact the optimal breaking of the cycle would be something like:
type.js -> namespace.js -> object.js -> util.js
(util.js thus executed first, type.js last)

@kskalski
Copy link
Author

kskalski commented Sep 2, 2021

From reading the code it seems that commonjs plugin processes modules one-file at a time, so it doesn't really have the ability to detect cycles during its execution. The breakup of cycles happens upstream in rollup itself (function analyseModuleExecution(entryModules) {), so I suppose this code should classify dependencies using some kind of signal, e.g. which references are strong and which are weak or maybe others too (e.g. heurisitcs like files with more inbound dependencies could be moved earlier in execution).

@kskalski
Copy link
Author

However the code generated by commonjs plugin begs for a question whether it should actually use dynamic import in cases where require is called inside a function (and conditionally). Right now for the original code

var roots = require("./roots");
...
util.decorateType = function decorateType(ctor, typeName) {
...
    if (!Type)
        Type = require("./type");
...
}

the plugin generates a top level imports block with

  import "./roots?commonjs-require";
  import require$$1 from "./roots?commonjs-proxy";
...
  import "./type?commonjs-require";
  import require$$5 from "./type?commonjs-proxy";

and require$$5 is used like this:

  util.decorateType = function decorateType(ctor, typeName) {
...
        if (!Type)
             Type = require$$5;

I suppose conversion of requires into imports should be done per scope (top-level vs functions) and for the latter might need to use dynamic import e.g.

  util.decorateType = function decorateType(ctor, typeName) {
      let require$$5 = null;
      import ("./type?commonjs-require").then(m1 => import("./type?commonjs-proxy").then(m2 => require$$5 = m2));
        if (!Type)
             Type = require$$5;
  }

and in that case the sorting of modules could better resolve the araising circular dependencies, since actually the static imports do not have a cycle here, it's only the dynamic imports that create them.

@kskalski kskalski changed the title Circular dependency is not resolved optimally by plugin-commonjs for protobufjs causing runtime error [@rollup/plugin-commonjs] Circular dependency is not resolved optimally by plugin-commonjs for protobufjs causing runtime error Sep 19, 2021
@boenfu
Copy link

boenfu commented Sep 24, 2021

I also encountered this problem when using Vite to build project. Is there any way to solve? ❤❤

@boenfu
Copy link

boenfu commented Sep 24, 2021

According to the prompted https://github.com/mmacfadden/rollup-protobufjs#notes
I alias to dist/protobuf.min.js to solved this problem.

// vite config
export default defineConfig({
  resolve: {
    alias: [
      {
        find: 'protobufjs',
        replacement: 'protobufjs/dist/protobuf.min.js',
      }
    ],
  }
});

@kskalski
Copy link
Author

I alias to dist/protobuf.min.js to solved this problem.

This is just switching to a subset of the library, which doesn't trigger the problem mentioned here. I was also using a protobuf-minimal import to avoid it in one of my projects. However now I need the full protobuf library, because I'm using grpc-js and proto-loader, which depend on the reflection part of protobufjs and they import field.js and object.js, which have conditional / dynami circular dependency.

@kzc
Copy link

kzc commented Oct 1, 2021

The example works if you replace the commonjs plugin with the alternative cjs plugin found at: rollup/rollup#4195 (comment)

Demonstration using a slightly more interesting test case:

diff --git a/index.js b/index.js
index 2383a47..a9e06a7 100644
--- a/index.js
+++ b/index.js
@@ -4,0 +5 @@ const root = protobuf.Root.fromJSON(awesome);
+toMessage("Hello").forEach(x => console.log(x, JSON.stringify(String.fromCharCode(x))));
$ rollup index.js -p node-resolve -p strict-cjs -p json -f cjs --exports auto --silent | node
(!) Error when using sourcemap for reporting an error: Can't resolve original location of error.
node_modules/@protobufjs/inquire/index.js?cjs (13:18)
10 "\n"
5 "\u0005"
72 "H"
101 "e"
108 "l"
108 "l"
111 "o"

Note that protobuf uses eval which prevents the minifier from performing a number of optimizations and name mangling. If you replace eval with the Function expression below you can achieve better minification of the final bundle without loss of functionality for this particular program:

Bundle size using rollup-plugin-terser without eval replace:

$ rollup index.js -p node-resolve -p strict-cjs -p json -f cjs --exports auto -p terser --silent | wc -c
(!) Error when using sourcemap for reporting an error: Can't resolve original location of error.
node_modules/@protobufjs/inquire/index.js?cjs (13:18)
   72260

Bundle size using rollup-plugin-terser and @rollup/plugin-replace with eval replaced by a Function expression:

$ rollup index.js -p node-resolve -p strict-cjs -p json -f cjs --exports auto -p terser -p replace='{eval:"(function(x){return new Function(\"return \"+x)()})"}' --silent | wc -c
   64602

Edit: corrected eval replace argument.

@kskalski
Copy link
Author

kskalski commented Oct 2, 2021

The example works if you replace the commonjs plugin with the alternative cjs plugin found at: rollup/rollup#4195 (comment)

@kzc it looks promising, any hint on how to solve errors like this (from my larger project) with the custom plugin? should I add aliases or something?

✓ 544 modules transformed.
[strict-cjs] Could not load electron?cjs (imported by node_modules\@electron\remote\dist\src\renderer\remote.js?cjs): ENOENT: no such file or directory, open 'electron'
error during build:
Error: Could not load electron?cjs (imported by node_modules\@electron\remote\dist\src\renderer\remote.js?cjs): ENOENT: no such file or directory, open 'electron'

@kzc
Copy link

kzc commented Oct 2, 2021

I've never used electron. It may be a native module. Try excluding it with ignore or exclude and rely on a dynamic require.

@kzc
Copy link

kzc commented Oct 2, 2021

It's also possible to use both @rollup/plugin-commonjs and rollup-plugin-strict-cjs in the same bundle by setting the include and exclude options appropriately for each plugin so they don't process the same directories. This might allow you to process electron with @rollup/plugin-commonjs if you previously had it working.

@kzc
Copy link

kzc commented Oct 4, 2021

I just verified that specifying native module name(s) in the the ignore option of rollup-plugin-strict-cjs works fine. In such cases the dynamic require is retained for the ignored modules - similar to @rollup/plugin-commonjs.

$ cat example.js 
var add = require("./add.js");
console.log(add(23, 100));
console.log(require("native-mod").Hello());
$ cat add.js
module.exports = function(x, y) { return x + y; };
$ node example.js
123
World
$ rollup example.js -p node-resolve -p strict-cjs='{ignore:["native-mod"],ignoreDynamicRequires:true}' --silent -f cjs --exports auto -p terser='{output:{beautify:true}}'
"use strict";

function o(o, e) {
    return o.__esModule ? o : (o.m || (o.m = {
        exports: e = {}
    }, o(e, o.m)), o.m.exports);
}

function e(o, e) {
    e.exports = function(o, e) {
        return o + e;
    };
}

var n = o((function(n) {
    var r = o(e);
    console.log(r(23, 100)), console.log(require("native-mod").Hello());
}));

module.exports = n;
$ rollup example.js -p node-resolve -p strict-cjs='{ignore:["native-mod"],ignoreDynamicRequires:true}' --silent -f cjs --exports auto -p terser='{output:{beautify:true}}' | node
123
World

@kskalski
Copy link
Author

kskalski commented Oct 4, 2021

Thanks, the options (ignore: ['electron']) for the plugin allowed me to compile my project successfully. I think I also got the runtime going past the originally reported cyclic dependency problem. :-)

It failed however on a different require scenario, which I am not yet able to decipher fully (i is not a Function at staticRequire). It failed on loading this module https://github.com/protobufjs/protobuf.js/blob/master/ext/descriptor/index.js (imported by this lib https://github.com/grpc/grpc-node/blob/master/packages/proto-loader/src/index.ts)
I will try to prepare an isolated reproduction later.

@kzc
Copy link

kzc commented Oct 4, 2021

Again, I don't know anything about these libraries, but it looks like grpc and grpc-node also use native modules and may need the same sort of ignore or externals workarounds.

https://github.com/grpc/grpc-node/blob/master/PACKAGE-COMPARISON.md

Have you tried using @grpc/grpc-js instead of @grpc/proto-loader ?

According to https://github.com/grpc/grpc-node/tree/master/packages/proto-loader documentation:

Usage

const protoLoader = require('@grpc/proto-loader');
const grpcLibrary = require('grpc');
// OR
const grpcLibrary = require('@grpc/grpc-js');

Out of curiosity, how was your system successfully built before? Webpack or esbuild?

@kskalski
Copy link
Author

kskalski commented Oct 5, 2021

I'm actualy using @grpc/grpc-js and there is no native part in this library. @grpc/proto-loader isn't either, it just uses the (previously failing due to circular dependency) parts of protobufjs to load the text .proto schema files into descriptor objects.

As for my build process - the circular dependency problem I know webpack handles properly to bundle the .js files, since I used it in one project before I switched it to Vite (by using a smaller dependency protobufjs-minimal which excludes the parts failing with rollup).

For the project experiencing this latest problem I don't bundle the files, since it's an Electron app that can just load individual files from disk on demand - I suspect webpack would still handle it well though.

@kskalski
Copy link
Author

kskalski commented Oct 5, 2021

So far an isolated test case using @grpc/proto-loader works fine for me in node using rollup-plugin-strict-cjs, so it might be something specific to the way electron loads the script or some other interaction with my larger project. I will try to reproduce it in self contained project.

It would be nice to have the proper handling of circular dependencies incorporated into plugin-commonjs, but in any case, thanks for letting this issue move forward. :-)

@kzc
Copy link

kzc commented Oct 5, 2021

The reason why I asked whether the system was successfully bundled before by any tool was to determine if there was any runtime introspection or monkey patching going on that would prevent successful operation. For example, the eval I mentioned above is used in a runtime require of Long.

@lukastaegert
Copy link
Member

We are finalizing a new version of the commonjs plugin in #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.

@kskalski
Copy link
Author

We are finalizing a new version of the commonjs plugin in #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.

@lukastaegert I tried to apply a change of

-    "@rollup/plugin-commonjs": "^21.0.0",
+    "@rollup/plugin-commonjs": "22.0.0-0",

to the reproduction repository https://github.com/kskalski/rollup-protobufjs, but the effect is that build step no longer produces any output without really emitting any errors (the step index.js → dist/index.umd.js... silently ceased to have any effects)

@lukastaegert
Copy link
Member

Yes, you also need to have the very latest node-resolve. I will include a check for that in the final version.

@kskalski
Copy link
Author

Ok, updating node-resole fixed the no-emit problem. However the old error caused by uninitialized object related to order of imports evaluation still remains.

I pushed packages update to the repo.

@kzc
Copy link

kzc commented Nov 29, 2021

The test repo appears to have been updated with the wrong version of the commonjs plugin. When this patch is applied to the test repo in https://github.com/kskalski/rollup-protobufjs and using the latest node-resolve plugin, the new commonjs plugin will produce output just 10% of the time. Non-determinisitic output is produced when it does work.

diff --git a/index.js b/index.js
index 2383a47..9a35b5e 100644
--- a/index.js
+++ b/index.js
@@ -25 +25,3 @@ export function toMessage(awesomeField) {
-}
\ No newline at end of file
+}
+
+console.log(JSON.stringify(Array.from(toMessage("Hello")).map(x=>[x,String.fromCharCode(x)])));
diff --git a/package.json b/package.json
index ed00ee3..415fd8d 100644
--- a/package.json
+++ b/package.json
@@ -17 +17 @@
-    "@rollup/plugin-commonjs": "22.0.0.0",
+    "@rollup/plugin-commonjs": "22.0.0-0",

Using esbuild:

$ esbuild index.js --bundle --format=esm | node --input-type=module
[[10,"\n"],[5,"\u0005"],[72,"H"],[101,"e"],[108,"l"],[108,"l"],[111,"o"]]

Using rollup-plugin-strict-cjs:

$ node_modules/.bin/rollup index.js -p node-resolve -p strict-cjs -p json --silent | node --input-type=module
[[10,"\n"],[5,"\u0005"],[72,"H"],[101,"e"],[108,"l"],[108,"l"],[111,"o"]]

(A rollup warning about the of the use of eval in protobufjs was omitted for brevity).

Using @rollup/plugin-commonjs@22.0.0-0 with default options - note the wc -c and shasum of the output:

$ for i in 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20; do echo; echo "$i:" ; rm -f out.mjs && node_modules/.bin/rollup index.js -p node-resolve -p commonjs -p json --silent -o out.mjs && wc -c out.mjs && shasum out.mjs && node out.mjs || echo FAIL ; done

1:
wc: out.mjs: open: No such file or directory
FAIL

2:
wc: out.mjs: open: No such file or directory
FAIL

3:
wc: out.mjs: open: No such file or directory
FAIL

4:
wc: out.mjs: open: No such file or directory
FAIL

5:
wc: out.mjs: open: No such file or directory
FAIL

6:
wc: out.mjs: open: No such file or directory
FAIL

7:
wc: out.mjs: open: No such file or directory
FAIL

8:
wc: out.mjs: open: No such file or directory
FAIL

9:
wc: out.mjs: open: No such file or directory
FAIL

10:
wc: out.mjs: open: No such file or directory
FAIL

11:
wc: out.mjs: open: No such file or directory
FAIL

12:
wc: out.mjs: open: No such file or directory
FAIL

13:
  234728 out.mjs
1c2821227578b91ade6b6dfa23c9ea5550e3b525  out.mjs
[[10,"\n"],[5,"\u0005"],[72,"H"],[101,"e"],[108,"l"],[108,"l"],[111,"o"]]

14:
  234407 out.mjs
606ee66de2294f659e603d1f22d9e05407ab679c  out.mjs
[[10,"\n"],[5,"\u0005"],[72,"H"],[101,"e"],[108,"l"],[108,"l"],[111,"o"]]

15:
wc: out.mjs: open: No such file or directory
FAIL

16:
wc: out.mjs: open: No such file or directory
FAIL

17:
wc: out.mjs: open: No such file or directory
FAIL

18:
wc: out.mjs: open: No such file or directory
FAIL

19:
wc: out.mjs: open: No such file or directory
FAIL

20:
wc: out.mjs: open: No such file or directory
FAIL

Using commonjs plugin with {strictRequires: true}:

$ for i in 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20; do echo; echo "$i:" ; rm -f out.mjs && node_modules/.bin/rollup index.js -p node-resolve -p commonjs='{strictRequires:true}' -p json --silent -o out.mjs && wc -c out.mjs && shasum out.mjs && node out.mjs || echo FAIL ; done

1:
wc: out.mjs: open: No such file or directory
FAIL

2:
wc: out.mjs: open: No such file or directory
FAIL

3:
wc: out.mjs: open: No such file or directory
FAIL

4:
wc: out.mjs: open: No such file or directory
FAIL

5:
wc: out.mjs: open: No such file or directory
FAIL

6:
  240190 out.mjs
aa15249fd34f8022caef93cfbc6efb7c76aee7ba  out.mjs
[[10,"\n"],[5,"\u0005"],[72,"H"],[101,"e"],[108,"l"],[108,"l"],[111,"o"]]

7:
wc: out.mjs: open: No such file or directory
FAIL

8:
  240190 out.mjs
aa15249fd34f8022caef93cfbc6efb7c76aee7ba  out.mjs
[[10,"\n"],[5,"\u0005"],[72,"H"],[101,"e"],[108,"l"],[108,"l"],[111,"o"]]

9:
wc: out.mjs: open: No such file or directory
FAIL

10:
  240190 out.mjs
aa15249fd34f8022caef93cfbc6efb7c76aee7ba  out.mjs
[[10,"\n"],[5,"\u0005"],[72,"H"],[101,"e"],[108,"l"],[108,"l"],[111,"o"]]

11:
wc: out.mjs: open: No such file or directory
FAIL

12:
wc: out.mjs: open: No such file or directory
FAIL

13:
wc: out.mjs: open: No such file or directory
FAIL

14:
wc: out.mjs: open: No such file or directory
FAIL

15:
wc: out.mjs: open: No such file or directory
FAIL

16:
wc: out.mjs: open: No such file or directory
FAIL

17:
wc: out.mjs: open: No such file or directory
FAIL

18:
wc: out.mjs: open: No such file or directory
FAIL

19:
wc: out.mjs: open: No such file or directory
FAIL

20:
wc: out.mjs: open: No such file or directory
FAIL

When the new commonjs plugin fails, rollup will exit entirely without errors or warnings and without any output, and with a 0 exit code - incorrectly indicating success. I was unable to isolate how an error escapes from being caught and reported. Before the bug in the plugin is fixed it would be helpful if rollup could detect such anomalous scenarios and report an error.

Edit: updated second example to correct a typo in the option name strictRequires. Based on a few runs, it appears that strictRequires: true produces deterministic output when it does work.

@kskalski
Copy link
Author

Thanks, I updated the repo following your patch and can confirm non-deterministic behavior of npm run build with commonjs plugin.

@lukastaegert
Copy link
Member

Thanks for spotting, there was a serious issue with the cycle detection logic. This should be fixed now, I released a new version 22.0.0-1. Please also look if there is still indeterminate output.

@kskalski
Copy link
Author

kskalski commented Dec 2, 2021

Thanks, the issue coming from circular dependency seems to be resolved with 22.0.0-1

@kskalski kskalski closed this as completed Dec 2, 2021
@kzc
Copy link

kzc commented Dec 2, 2021

So far an isolated test case using @grpc/proto-loader works fine for me in node using rollup-plugin-strict-cjs, so it might be something specific to the way electron loads the script or some other interaction with my larger project. I will try to reproduce it in self contained project.

@kskalski I noticed that kskalski/rollup-protobufjs@d985b9f in your test case also works with rollup-plugin-strict-cjs.

Just curious... does @rollup/plugin-commonjs@22.0.0-1 have the same issues in your electron app as rollup-plugin-strict-cjs - i.e., a non-plugin problem?

@kskalski
Copy link
Author

kskalski commented Dec 2, 2021

Indeed an extended test case with using @grpc/proto-loader (kskalski/rollup-protobufjs@d985b9f) does work fine with @rollup/plugin-commonjs@22.0.0-1 too.
However I stumbled upon another issue when trying to also use @grpc/grpc-js, which I reported in #1059 (repro is a new branch in https://github.com/kskalski/rollup-protobufjs/tree/grpc)

At this point I didn't test the new plugin version in my original Electron project, since the problem appears in a standalone case with just trying to import @grpc/grpc-js (note that this library requires node environment and running it in browser is not supported, which might be also true for @grpc/proto-loader, but bundling it with rollup now works and output is runnable by node dist/index.umd.js).

@kzc
Copy link

kzc commented Dec 2, 2021

I can confirm that https://github.com/kskalski/rollup-protobufjs/tree/grpc works with rollup-plugin-strict-cjs:

$ rm -f out.js && node_modules/.bin/rollup index.js -p node-resolve -p strict-cjs -p json --silent -o out.js -f cjs --exports auto && wc -c out.js && shasum out.js && node out.js || echo FAIL
  703338 out.js
a5e14c906cb2e4c54cb8b35ea613a777cdf1859f  out.js
[[10,"\n"],[5,"\u0005"],[72,"H"],[101,"e"],[108,"l"],[108,"l"],[111,"o"]]
package {
  Foo: {
    format: 'Protocol Buffer 3 DescriptorProto',
    type: {
      field: [],
      nestedType: [],
      enumType: [],
      extensionRange: [],
      extension: [],
      oneofDecl: [],
      reservedRange: [],
      reservedName: [],
      name: 'Foo',
      options: null
    },
    fileDescriptorProtos: [
      <Buffer 0a 0a 72 6f 6f 74 2e 70 72 6f 74 6f 22 05 0a 03 46 6f 6f 62 06 70 72 6f 74 6f 33>
    ]
  }
}
finished

but fails with @rollup/plugin-commonjs@22.0.0-1:

$ npm ls --silent | egrep 'commonjs|node-resolve'
├─┬ @rollup/plugin-commonjs@22.0.0-1
├─┬ @rollup/plugin-node-resolve@13.0.6
$ rm -f out.js && node_modules/.bin/rollup index.js -p node-resolve -p commonjs -p json --silent -o out.js -f cjs --exports auto && wc -c out.js && shasum out.js && node out.js || echo FAIL
wc: out.js: open: No such file or directory
FAIL

strictRequires: true doesn't appear to help:

$ rm -f out.js && node_modules/.bin/rollup index.js -p node-resolve -p commonjs='{strictRequires:true}' -p json --silent -o out.js -f cjs --exports auto && wc -c out.js && shasum out.js && node out.js || echo FAIL
wc: out.js: open: No such file or directory
FAIL

This new @rollup/plugin-commonjs@22.0.0-1 error happens 100% of the time, unlike the last error which was indeterminate.

@lukastaegert Is there any way you can detect when a plugin causes rollup itself to exit without error or warning and produce no output? process.exit is not being called by the plugin in this case. Perhaps some undetected Promise error is not being caught by rollup?

@lukastaegert
Copy link
Member

@lukastaegert Is there any way you can detect when a plugin causes rollup itself to exit without error or warning and produce no output

I fear this is not possible. The reason is that the event loop is basically exhausted while the build is not done, there is no error involved. Basically you have promises waiting for each other in a circular way. This can happen if you use a this.load within a module transformer and you have a cyclic dependency.

This is something the plugin actually does, but I thought I finally have the cycle detection figured out, obviously this is not the case. Maybe I need to rethink the entire algorithm here. The previous issue was that I had assumed that the modules in a cycle are loaded in the "right" order, but this is certainly not the case. Will need to see what the new issue is about.

Another point to fix is that the cycle detection is always running while it should only be running when strictRequires is "auto".

@kzc
Copy link

kzc commented Dec 4, 2021

Do you think this plugin is reaching a fundamental limit of in-flight Promises causing NodeJS to exit unexpectedly? Or is it just a case of NodeJS will exit when there are no more pending events to process? If the latter, perhaps an artificial pending event can be introduced to gate the process.

@lukastaegert
Copy link
Member

Or is it just a case of NodeJS will exit when there are no more pending events to process

I think this is the case. What we could do is indeed introduce something timeout based, but this will not solve the underlying issue, in the best case it will just "hang" instead of exiting.

@lukastaegert
Copy link
Member

I think I fixed the issue now. I assume it was caused by some premature optimizations I built into the original algorithm. Instead of trying to debug this, I switched now to a much simpler cycle detection algorithm that does not try to cache too much. I worked fine for the example given (and also in a complex project of my own). Published as @rollup/plugin-commonjs@22.0.0-2 and also updated the "beta" tag.

@Paul-Hebert
Copy link

We are finalizing a new version of the commonjs plugin in #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.

@lukastaegert I just wanted to chime in and mention that the beta version of the commonjs plugin fixed a circular dependency issue for me!

I was attempting to build a third-party React package with Rollup. The package uses styled-components, and it was crashing when it hit circular dependencies. I was able to build the package with webpack but not Rollup. Updating to the beta version allowed me to build the package with Rollup.

Thanks for the great work!

@dantman
Copy link

dantman commented Apr 9, 2022

I'd also like to chime in with another example. I tried switching a CRA project to Vite and encountered it breaking in the production build because of this bug happening to xmlbuilder.

https://github.com/dantman/rollup-commonjs-circular-xmlbuilder-bug

I tested it with the beta and it fixed the issue. So I am encouraged there is a solution to it and ~~all it requires is Vite to update a package.

However, I am extremely discouraged by the fact that this confirmed fix for a bug that makes rollup unusable to users of certain libraries has been "beta" for 3.5 months. I can't report this as a bug and ask them to bump the package, because the package doesn't even have an official release. And because it's a major and the dependency is indie Vite I can't even workaround this by installing the beta in my package as Vite will continue to use the older version.

Edit: It appears that Vite doesn't actually use @rollup/plugin-commonjs this may be a bug in vite or esbuild, I got confused by the fact that Vite does use this package as a dev dependency and I can reproduce the exact same error with a minimal rollup reproduction.

Edit2: I retract my retraction. Vite does use @rollup/plugin-commonjs, it's just compiled into the vite package using rollup. So it's not even possible to use something like patch-package to replace the @rollup/plugin-commonjs version. Additionally I tried out dynamicRequireTargets which appears to be the official workaround. However it doesn't work at all as adding xmlbuilder to dynamicRequireTargets results in jsx-xml (the middle package that actually uses xmlbuilder) having a "Could not dynamically require" error, and adding both xmlbuilder and jsx-xml leads to either a build time error (because the named imports are now on a default import object) or a "require is not defined" error if I fix that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants