web: [@web/dev-server]: `@rollup/plugin-commonjs` version `>18` breaks commonjs imports

I have the same problem with @rollup/plugin-commonjs@22.0.0. I’m getting this error message:

Error while transforming node_modules/@dynatrace/openkit-js/dist/node/index.js: Could not resolve import "commonjsHelpers.js".
> 1 | import * as commonjsHelpers from "commonjsHelpers.js";

Using @rollup/plugin-commonjs@18.0.0 the errors becomes:

Error while transforming node_modules/axios/package.json: Unexpected "export"

web-test-runner.config::plugins:

json(),
nodeResolve(),
commonjs({ sourceMap: false }),
nodePolyfills(),
esbuildPlugin({ ts: true, js: true, json: true }),

I added the new version that I needed with:

npm install -D @web/dev-server-rollup@0.0.0-canary-20230216020153

It’s working great so far, thanks y’all!

@tannerstern I think that #2050 is likely what you need - this is the pull request that will hopefully fix the broken CommonJS support in general. On the other hand, #2103 addresses an additional Windows specific issue (due to absolute file paths starting with things like C:).

i’ve been working on it in #2050 to fix it at the source (in modernweb).

however i have yet to solve the infinite loop (supporting skipSelf properly). have spent a fair chunk of time staring at that code and trying to figure out how to solve it, it’ll come to me eventually.

its very poorly documented in rollup and very few (if any) examples exist.

once i solve that, that PR will fix everything in this issue.

@lucaelin i think i know now how we can implement all of this in wds itself

the only thing missing is the fix on rollup’s end for the import/export mismatch

ill add a draft PR and add you as a reviewer in case you have any better ideas

The problem still exists. Installing the @rollup/plugin-commonjs@22.0.0-4 (beta) did not help but produces another error: Error while transforming node_modules/axios/index.js: Could not resolve import "commonjsHelpers.js".

Having the same issue with jsondiffpatch package using vite. First, I thought it’s vite issue, but then I saw they updated the commonjs dependecy which breaks it. You can check the two reproduction links: https://github.com/vitejs/vite/issues/11986

@lucaelin To follow up:

1. Turns out it was a Windows specific issue, that coincidentally was just reported by someone else in #2078 (a fix was attempted in #2079, but has since been reverted)
2. With the latest version (24.0.0) of @rollup/plugin-commonjs, I have found that I do not need fixExportNamedExports from the monkey patch. In fact, including it was causing errors. I think this was resolved by rollup/plugins#1363

@lucaelin Thank you - this is already helpful. I will try running my setup on a Mac and see if the problem is still there. In code with this much path manipulation, it could certainly be a Windows-specific issue. If I do still see the problem, then I will try to create a minimal example that reproduces the issue, so that I can share. Thanks again.

@lucaelin, thank you for helping with this issue. I’m trying to use your monkey-patch with web-test-runner, but I’m now getting the error: SyntaxError: Octal escape sequences are not allowed in strict mode.

When I run web-test-runner in manual mode and look at the source of the error in Chrome I see:

import * as commonjsHelpers from "/__web-dev-server__/rollup/commonjsHelpers.js?web-dev-server-rollup-null-byte=%00commonjsHelpers.js";
import { __module as deepEqlModule, exports as deepEql } from "**NUL**C:\akrawitz\Synchronized\Research - Victoria\Misc\decidables\libraries\detectable-math\__wds-outside-root__\2\node_modules\deep-eql\index.js?commonjs-module"
import require$$0 from "/__web-dev-server__/rollup/type-detect.js?web-dev-server-rollup-null-byte=%00C%3A%5Cakrawitz%5CSynchronized%5CResearch%20-%20Victoria%5CMisc%5Cdecidables%5Cnode_modules%5Ctype-detect%5Ctype-detect.js%3Fcommonjs-proxy";

Except that where you see **NUL** in the second line, there is an actual NUL character, which I assume is what is causing the error.

As I understand it, these NUL characters are an internal signal for Rollup plugins, but then I would assume that they shouldn’t be making it into the code that gets served to the browser.

Any thoughts on why this is happening?

I don’t think that this is true, because then my patch would not work. The commonjsHelpers.js import is also prefixed with an invisible null-byte, telling node-resolve to skip resolving it and handing it over to the commonjs plugin.

yeah sorry, i meant the rollupAdapter.

it goes unresolved so reaches this

So is it basically:

• Fix the rollupadapter here to assign the return value - you can already see the ?? rollupInputOptions is meaningless (since it isn’t a call to anything and isn’t assigned to anything), so my guess is the original author meant to assign it but didn’t
• Fix @rollup/plugin-commonjs on their end since the import/export don’t match up and clearly should (unless somehow we’re not looking at the right import and/or export, i.e. the right pair)
• Also fix in the rollupAdapter the fact it doesn’t respect skipSelf - weirdly this does look like it should already work, would be good to know for sure why it doesn’t