-
-
Notifications
You must be signed in to change notification settings - Fork 293
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
[@web/dev-server]: @rollup/plugin-commonjs
version >18
breaks commonjs imports
#1700
Comments
@rollup/plugin-commonjs
version >18 breaks commonjs imports@rollup/plugin-commonjs
version >18
breaks commonjs imports
const basename = process.env.NODE_ENV === 'production' ? '/my-react-app' : '/'; return ( |
The problem still exists. Installing the @rollup/plugin-commonjs@22.0.0-4 (beta) did not help but produces another error: |
I seem to have hit this as well. I was trying to run tests on code that uses jStat. Before I added
Fair enough, since jStat is only provided in UMD format, I added the CommonJS plugin to my Web Test Runner config: import rollupPluginCommonjs from '@rollup/plugin-commonjs';
import { fromRollup } from '@web/dev-server-rollup';
const commonjsPlugin = fromRollup(rollupPluginCommonjs);
export default {
nodeResolve: true,
plugins: [
commonjsPlugin({
include: [
'../../**/node_modules/jstat/**/*',
],
}),
],
}; However, this led to me getting the following error when trying to run tests:
For the life of me, I couldn't figure out what was using octal escape sequences. I finally stumbled on this issue, and after pinning I'm posting this here in case it helps others realize that what seems like a completely different issue may in fact have this same cause. EDIT: |
I have the same problem with
Using
web-test-runner.config::plugins:
|
Unfortunately, I haven't found a proper solution fo the problem. What I did to bypass/evade the problem is using rollup-plugin-url-resolve to pull the package I need as a module (or transformed into a module) from unpkg and include it into the build. Due to privacy/security concerns, loading it from a (public) cdn (in production) wasn't an option. In my source file:
In my rollup.config.js:
I know it isn't a 'proper' fix and might not solve everybody's problems, but hope that it helps some of you. |
Ended up bundling this particular library dependency and handling it separately. Here's how this looks. The dependency in question is a dependency of my dependency and this works. // for the messy library only
import { nodeResolve } from '@rollup/plugin-node-resolve';
import commonjs from '@rollup/plugin-commonjs';
export default {
input: './lib/entry.js',
// output: {
// file: './__test/lib.bundle.js',
// format: 'module',
// },
treeshake: false,
output: [{
file: "lib/lib.bundle.js",
format: "esm",
// preserveModules: true
}],
plugins: [nodeResolve(), commonjs()]
}; lib/entry.js import 'deepmerge/index'; /* eslint-disable */
import { storybookPlugin } from '@web/dev-server-storybook';
import { hmrPlugin, presets as hmrPresets } from '@open-wc/dev-server-hmr';
import rollupCommonjs from '@rollup/plugin-commonjs';
import { fromRollup } from '@web/dev-server-rollup';
import rollupAlias from '@rollup/plugin-alias';
import resolve from '@rollup/plugin-node-resolve';
import * as path from 'path';
const pluginCommonjs = fromRollup(rollupCommonjs);
const pluginAlias = fromRollup(rollupAlias);
export default {
port: 8000,
nodeResolve: {
resolveOnly: [
/^(?!.*(deepmerge|different_npm_library))/,
],
},
plugins: [
pluginAlias({
entries: [
{
find: 'deepmerge',
replacement: `${process.cwd()}/lib/lib.bundle.js`,
},
],
}),
storybookPlugin({ type: 'web-components' }),
],
}; |
I'm facing the same problem when using https://www.npmjs.com/package/slugify 1.6.5 in my ts project with
Opening the webpage spits this error in console:
Unfortunately, downgrading |
@daKmoR any idea what to do here? i understand downgrading solves the problem, but this isn't obvious from any of your docs, examples, etc. currently someone (like i did) might think "ok ill go install dev-server-rollup and plugin-commonjs, then job done". e.g. export default {
// ...
plugins: [
esbuildPlugin({ ts: true, target: 'auto' }),
fromRollup(commonjs)()
]
}; but this won't work out of the box because of this issue (some nonsensical import in plugin-commonjs it seems). similarly, maybe its worth you having docs on using commonjs in particular in WTR/dev-server? unless im just missing it, i can't seem to find a page explaining it. i had to figure it out from someone else's repo. edit: actually downgrading doesn't solve anything, i get the same inf loop balusc gets edit2: it seems the latest one falls over because this flow happens:
in the commonjs plugin, they tell rollup to return/inject some fabricated module anytime something tries to import their special helpers name: |
I managed to monkey-patch the commonjs module to now properly resolve the |
Trying to minimally reproduce the issue I had above, I accidentally managed to reproduce the infinite loop problem first. |
I now found a solution for the third issue, regarding All in all I created a set of monkey-patches you can load into the web-dev-server config and released them here. I hope to soon create a PR for this project and start a discussion on how to fix the remaining issue in the commonjs plugin. |
@lucaelin maybe several issues at once. the error about exports is unlikely to be from the helpers module. this doesn't load because the node-resolve module executes first in WTR/dev-server and decides the import is invalid (unresolvable) before we even reach the commonjs plugin code. i guess thats why they prepend the plugin you mentioned, to reach it first. seems we would have to fix plugin-commonjs since it is producing nonsensical code (the non-existent export). |
Yes, it's three issues and they are all interconnected. Afaict the non-existing export is the only one within the commonjs plugin itself, it just doesn't seem to surface when bundling with rollup. I've reported that one here.
I don't think that this is true, because then my patch would not work. The |
yeah sorry, i meant the rollupAdapter. it goes unresolved so reaches this So is it basically:
|
@lucaelin we can possibly solve the first one by updating the let result = null;
if (rollupInputOptions.plugins) {
for (const subPlugin of rollupInputOptions.plugins) {
if (subPlugin.resolveId) {
result = await subPlugin.resolveId.call(
rollupPluginContext,
resolvableImport,
filePath,
{isEntry: false}
);
if (result !== null) {
break;
}
}
}
}
if (result === null) {
result = await rollupPlugin.resolveId?.call(
rollupPluginContext,
resolvableImport,
filePath,
{isEntry: false}
);
} the way we call out to rollup means we don't really have a real context (i.e. any plugins). we have the one plugin we're wrapping and nothing else, so there's nowhere useful we can put the mutated options since we won't consume them again after creating the initial context. this is why i had to instead call out to the plugins. though we could at least wrap that into some helper function this then results in the infinite loop - but the docs are so vague and terrible around |
As I mentioned above:
Without doing this, node-resolve sets the skipSelf flag and forwards the resolution to commonjs, which in turn sets the skipSelf flag. Since the adapter doesn't remember that node-resolve previously already set the flag, it runs node-resolve again, which again sets the skipSelf flag, forwarding it back to commonjs... and so on. |
so basically afaict every wds plugin gets its own rollup context, has no idea the other plugins even exist. so we'd have to share the cache across wds plugins somehow. the only thing they really share is the wds config/context, maybe we'll have to mutate that 🤔 that makes sense though:
is it as simple as that? |
@43081j I've update the gist, because your notes above gave me an idea. Now instead of skipping the resolve loop entirely, I move the burden of checking the skipSelf back into the module itself. The patched module now gets its own set of files that it has previously called resolve with skipSelf with. Upon calling the resolveId function, the plugin now checks against its own set of files and skips its further execution, in turn preventing the loop. |
i had a go at doing similar in the dev-server but realised you can't really prepend any plugins since each one runs individually. curious how your patch manages to work in that case, i must be missing something. even with an updated rollup adapter that calls sub-plugins, node-resolve will have already run by then and thrown its exception (it doesn't pass through, the adapter throws if its resolveId failed to resolve). maybe if you set |
@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 |
I tried the workaround in the gist in vaadin/hilla@f423083 Now the tests fail with errors like
and also
as you can see here https://github.com/vaadin/hilla/actions/runs/3234340457/jobs/5297380520 |
Those are nodejs built-in modules and not available in browsers by default. You will need to add additional plugins to your config to provide those.
well that was just sloppiness on my part :D I check if the URL for |
@web-padawan i roughly understand it and have most of it in #2050 but could do with help getting it completed it is an incomplete solution right now. i got a little stuck and preoccupied with other things since so its gone a bit stale. if you want to catch up on twitter dm / discord about it i'd be up for that, if you're interested in helping me sort it out. i still do want to fix it (and need to, really). its just a tough one since nobody else seems to exist other than lucaelin who understands any of it 😂 |
@akrawitz I've attempted to reproduce the issue, but it seem to work just fine on my end. Could you tell me which line you are getting the original syntax-error on? And also a list of all the plugins you are using on wds would be helpful. |
@Lodin you recently mentioned that this issue makes WTR unusable for |
@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 To follow up:
|
Having the same issue with |
This issue is stalling our switch to And unfortunately for us, switching back to This is all in a linux environment (GitHub codespaces actually). I was hopeful that the change in #2103 would help, and I tried adding that change in myself locally, but it didn't seem to help. |
@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 |
I just cloned @43081j's fork and switched to his PR branch (from #2050) and copied the built version of @akrawitz thanks for putting me on to this! I see it was marked "Ready for Review" just an hour ago, excited for it to be included in an upcoming version! |
For those of you interested, we're doing some testing against a canary release in this area via:
Any feedback on it working or not would be great! |
I added the new version that I needed with:
It's working great so far, thanks y'all! |
@Westbrook +1 here as well. Upgrading to canary version fixed the |
@Westbrook I can confirm that the canary version is working for me as well, but only if I also apply the fix for Windows environments in #2103. |
@Westbrook I see |
Mostly working but blocked as per modernweb-dev/web#1700 (comment)
Still hitting the same bugs here. Test environment is:
I am testing with an import of the legacy With With This reports as a failure to load module source for this URL and these two others, which are presumably its immediate dependants. http://localhost:8000/__web-dev-server__/rollup/nacl-fast.js?web-dev-server-rollup-null-byte=%00%2Fhome%2Fpospi%2Fprojects%2Fneighbourhoods%2Fapplet-template%2Fnode_modules%2F.pnpm%2Ftweetnacl%401.0.3%2Fnode_modules%2Ftweetnacl%2Fnacl-fast.js%3Fcommonjs-module I have commonjs({
include: [
'**/node_modules/tweetnacl/*',
],
}), in my WDS |
...just re-tested with |
@pospi If those actually are dependents, then you're likely not leveraging the API for the Rollup Plugin as expected. Take a look at https://github.com/modernweb-dev/web/pull/2180/files#diff-71859c8cb2f330cca436fcf08f2826397cae98029c3b795a23b4b05703d56e06R207 where our test suite outlines this. The |
Tried it as suggested ( Do I manually need to resolve this module myself? Why is this string present literally nowhere on Github let alone here or the Rollup repos and why are the Tutanota team giving it special handling? 😂 |
@pospi I'm not sure if this will be helpful or not, but two thoughts:
|
Does that mean there's a different existing issue I should be filing these logs under, or should I submit a new one? "@rollup/plugin-commonjs": "^24.0.1",
"@rollup/plugin-replace": "^5.0.2",
"@web/dev-server": "^0.1.38",
"@web/dev-server-esbuild": "^0.3.6",
"@web/dev-server-rollup": "^0.4.0",
"rollup": "^3.20.2", |
I'm running into this issue of
Note: When using "use strict";
module.exports = require('ws'); to esm notation "use strict";
export default 'ws'; |
I think this bug might be resolved with Can somebody confirm that? |
Consuming dependencies that have to be transformed using the
@rollup/plugin-commonjs
plugin break when version>18
of the plugin is used.I have a simple example repository: https://github.com/peschee/wds-commonjs-example
In the
main
branch, trying to run wds (npm run start
) shows the following error when importingapexcharts
ormoment
(those are the ones I tested):I also have a simple rollup build in the repo, using the same plugins as in wds, and that one works (
npm run build
).Downgrading
@rollup/plugin-commonjs
to^18
makes the imports work again in wds: https://github.com/peschee/wds-commonjs-example/pull/1/filesThe text was updated successfully, but these errors were encountered: