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

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!

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.

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.

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',
      }
    ],
  }
});

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.