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.

[lukastaegert]

Admittedly I am not sure why node-resolve would interfere here. In any case, there is a pattern to protect against over-eager resolveId hooks: Use the options hook to inject a "child-plugin" in position one that contains your resolver. The commonjs plugin does this now as well specifically to work around issues with node-resolve:

options(rawOptions) {
      // We inject the resolver in the beginning so that "catch-all-resolver" like node-resolver
      // do not prevent our plugin from resolving entry points ot proxies.
      const plugins = Array.isArray(rawOptions.plugins)
        ? [...rawOptions.plugins]
        : rawOptions.plugins
        ? [rawOptions.plugins]
        : [];
      plugins.unshift({
        name: 'commonjs--resolver',
        resolveId
      });
      return { ...rawOptions, plugins };
    }

[lukastaegert]

Maybe at some point we should take a leaf out of Vite's book and add our own "enforce: 'pre'" option

[lukastaegert]

#1180 will avoid proxying for non-cjs entry points, I think that will make everyone happier

[Pauan]

That's great news, though it doesn't fix the specific issue with rollup-plugin-rust, since that issue is with node-resolve, not commonjs.

Using options to override the plugin order seems incredibly hacky, I think we should brainstorm a more robust solution. Plugins should be able to proxy entries without screwing over other plugins.

[lukastaegert]

While the "hack" would give you a way to solve the issue independently and quickly (and I do not think it is really terrible: Sometimes you just really want a hook to tun first or last and need to override ordering by users). Still, here is my proposal for a long-term solution in node-resolve:

#1181

If you can verify that this solves the issues, that would help in getting it merged.

[Pauan]

@lukastaegert Sorry for the delay. Although that PR does fix the immediate issue, I have an idea:

Right now the resolve method returns an object with various information. What if Rollup added a new property to that? So the resolve method would return something like this:

{
    id: string,
    external: boolean | "absolute",
    moduleSideEffects: boolean | 'no-treeshake',
    syntheticNamedExports: boolean | string,
    meta: {[plugin: string]: any},
    resolvedBy: string | null,
}

The new resolvedBy property would be the name of the plugin which resolved it. If all of the plugins return null then resolvedBy will be null.

So instead of checking the id, the node-resolve plugin would check the resolvedBy property: if it's null then it knows that the other plugins didn't resolve it.

The advantage is that this will work even if a plugin doesn't change the id, but instead changes other settings (like external, or meta, or moduleSideEffects).

This is essentially the same as enforce: "post", since it means that node-resolve will only resolve the ids if no other plugin resolves it first.

[lukastaegert]

Sounds like an idea. PR welcome if you want to speed up things.

[luckylooke]

Hi guys,
I hit same error:

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

solved by downgrading @rollup/plugin-commonjs to version 21 🤷 🙂

[joshuakb2]

I'm having the same error as @luckylooke. I followed the recommendation and added .ts?commonjs-entry as an extra file extension in the eslint config file. Now I get this error message:

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

No matter what I do, I can't seem to get the eslint plugin to agree that .ts?commonjs-entry files are included in the project.