CommonJS plugin breaks isEntry for entry points

[Pauan]

• Rollup Plugin Name: @rollup/plugin-commonjs
• Rollup Plugin Version: 22.0.0
• Rollup Version: 2.70.2
• Operating System (or Browser): Linux 64-bit NixOS
• Node Version: v16.14.0
• Link to reproduction (⚠️ read below): https://replit.com/@Pauan1/rollup-plugin-repro#rollup.config.js

Expected Behavior

The console should contain the following:

/home/runner/rollup-plugin-repro/input.js true

Actual Behavior

The console contains the following:

/home/runner/rollup-plugin-repro/input.js false
/home/runner/rollup-plugin-repro/input.js?commonjs-entry true

This means that the entry point is returning isEntry: false even though it should return isEntry: true

Additional Information

I noticed this bug while using the Rust Rollup plugin. With that plugin, you can import Cargo.toml files:

export default {
    input: {
        foo: "Cargo.toml",
    }
};

The Rust plugin requires isEntry in order to generate correct code.

However, even though those Cargo.toml files are not CommonJS (they're not even JS!) the commonjs plugin breaks them by causing isEntry to return false even though it should return true.

[lukastaegert]

Unfortunately, I could not get the plugin to work easily without proxying all entry points. In what way does the rust plugin rely on the toml file being marked as an entry? Could it be solved in another way?

[lukastaegert]

This is also a more general question as proxying entry points is a pattern that other plugins could use as well, I even promote it as a pattern for polyfill injection in the docs, but this should not break your plugin.

[Pauan]

What is the point of having an isEntry if it doesn't actually reliably work for detecting the entry? That just makes isEntry completely useless.

The Rust plugin uses isEntry because it loads Wasm files. Those Wasm files have to be loaded asynchronously, so they have to use Promises.

You can see the code here: if the file is an entry, then it immediately loads the Wasm promise and uses .catch(console.error); to catch any errors.

However, if the file is not an entry, it returns a function which must be called by the importer to load the Wasm.

In other words: if it's not an entry then it gives a function which will load the Wasm file asynchronously (via promises). But if it's an entry then it must automatically load the Wasm file, it cannot return a function.

This is the behavior that users expect: if they're importing the Rust program as an entry point, then it should automatically run the Rust program (just the same as a JS program).

[Pauan]

The actual wrapper isn't the problem (input.js?commonjs-entry), the issue is that it changes input.js so that it's no longer an entry, even though it was previously an entry.

Why can't the CommonJS plugin just ensure that input.js stays as an entry? Perhaps by overwriting isEntry?

Also, why does the CommonJS plugin wrap non-.js files? Since the Rust plugin uses Cargo.toml files, those shouldn't be touched at all by CommonJS, but they are being touched.

[silkfire]

After upgrading to v22.0.0, I'm no longer able to build; getting an error similar to No files matching <FILENAME>?commonjs-entry were found.

[privatenumber]

Likely because you have a plugin that relies on the module id (eg. chunk.facadeModuleId), and @rollup/plugin-commonjs@22.0.0 started appending queries to it.

Either the other plugins need to strip queries, or @rollup/plugin-commonjs needs to strip or stop appending it.

[silkfire]

Yea it's actually the eslint plugin (@rbnlffl/rollup-plugin-eslint). Should I report this as a bug? Or is there a temporary workaround?

[shellscape]

I thought we'd moved that plugin here.

[silkfire]

Ah, I didn't realize there's actually an official plugin for eslint over here. That plugin seems to be regularly updated too, not sure whether it's needed then.

Anyway, I replaced that plugin with the official one and got this error:

  0:0  error  Parsing error: "parserOptions.project" has been set for @typescript-eslint/parser.
The file does not match your project config: <PATH>\<FILE>.ts?commonjs-entry.
The extension for the file (.ts?commonjs-entry) is non-standard. You should add "parserOptions.extraFileExtensions" to your config

edit:

After adding the extraFileExtensions options I'm getting this error, which I cannot seem to resolve:

  0:0  error  Parsing error: "parserOptions.project" has been set for @typescript-eslint/parser.
The file does not match your project config: <PATH>\<FILE>.ts?commonjs-entry.
The file must be included in at least one of the projects provided

I tried adding .ts?commonjs-entry in the include option of tsconfig.json but I believe it doesn't accept file extensions with ? as a valid value.

[lukastaegert]

I will see if I can reduce entry proxying. In any case @Pauan your solution is still fragile in so far as there is a race condition depending on whether a .toml file is first imported as an entry point or first imported as a dependency. isEntry is unfortunately notoriously unreliable for load and transform as modules can be promoted to entry points at a later time via this.emitFile. Still, if isEntry is true we have an entry point.

A better solution, though, is to hook into resolveId instead and inject a proxy yourself. For the resolveId hook, isEntry IS reliable. It could work like this (not tested, please give it a go):

const entrySuffix = '?rust-entry';

// ...

function rust() {
  // ...
  return {
    // ...

    async resolveId(id, importer, options) {
      if (options.isEntry && $path.basename(id) === "Cargo.toml") {
        // turn into absolute path the same way Rollup would resolve it
        const resolvedId = $path.resolve(id);
        return `${resolvedId}${entrySuffix}`;
      }
      // ... other code
    }

    load(id) {
      if (id.endsWith(entrySuffix) {
        return `import wasm from ${JSON.stringify(id.slice(0, -entrySuffix.length))}; wasm();`
      }
      // ... other code
    }
  }
}

That way, it should even be possible to use the same toml file as both an entry point and a dependency in the same graph and the "right" thing should happen depending on whether the entry point is called first or it is first used as a dependency.

Depending on how you do it, this could actually make your code nicer in other places as that way, you can unify the logic of entry points and non-entry points.

[jakearchibald]

fwiw, this just caught me out too. I had a plugin that wasn't expecting the query to appear at the end of the facadeModuleId .

[Pauan]

@lukastaegert For the resolveId hook, isEntry IS reliable.

That's good to know, I will use that solution, thanks.

[Pauan]

@lukastaegert I changed rollup-plugin-rust so that it uses resolveId + load instead of transform. This does fix the isEntry problem, and now it works properly even if a Cargo.toml file is loaded multiple times.

However, there is still one issue: if the user does this, then everything is perfect...

export default {
    plugins: [
        rust(),
        nodeResolve(),
        commonjs(),
    ],
};

...but if the user does this instead...

export default {
    plugins: [
        nodeResolve(),
        commonjs(),
        rust(),
    ],
};

...then everything seems to work okay (no warnings or build errors), but at runtime everything is broken because nodeResolve has a resolveId which is running before rust's resolveId, and that resolveId screws up the entries.

So why is the nodeResolve plugin changing non-.js files? Shouldn't it just leave the poor Cargo.toml file alone?

This is a horrible user experience: if the user puts the plugins in the wrong order then they get no warning or build error, but things mysteriously break at runtime. And they break in a really subtle way which is hard to debug.

I know this is an issue with nodeResolve and not commonjs, but it seems to be a part of a general pattern of plugins not having a good way to preserve isEntry for other plugins. If proxies are supposed to be a common pattern, then the proxies need to not break isEntry.