Avoid collecting nodejs_compat warnings during dependency optimization#12381
Avoid collecting nodejs_compat warnings during dependency optimization#12381petebacondarwin merged 4 commits intomainfrom
nodejs_compat warnings during dependency optimization#12381Conversation
🦋 Changeset detectedLatest commit: 1381188 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Claude finished @jamesopstad's task —— View job Changeset ReviewTodo List
✅ All changesets look good The changeset in
The changeset provides a good explanation of the issue (warnings only shown when dependencies changed, false positives) and the solution (externalize Node.js built-ins during optimization, collect warnings at runtime). |
create-cloudflare
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
| test("displays warnings if Node.js built-ins are imported and the `nodejs_compat` flag is not enabled", async () => { | ||
| const proc = await runLongLived("pnpm", "dev", projectPath); | ||
| expect(await proc.exitCode).not.toBe(0); | ||
| const errorLogs = proc.stderr.replaceAll("\\", "/"); |
There was a problem hiding this comment.
Does this + checking the link has real added value?
There was a problem hiding this comment.
I'm not sure what this means.
| handler(source, importer) { | ||
| return resolveId(this.environment.name, source, importer); | ||
| // Fallback for when filter is not applied | ||
| // TODO: remove when we drop support for Vite 6 |
There was a problem hiding this comment.
suggest: TODO -> create a GH issue and link it here
There was a problem hiding this comment.
This didn't change in this PR (the code was just moved) and we have a lot of these sprinkled around so it would add noise to change them here. I'm creating tickets for them.
| nodeJsCompatWarningsMap.get(workerConfig); | ||
| nodeJsCompatWarnings?.registerImport(source, importer); | ||
|
|
||
| // Mark this path as external to avoid messy unwanted resolve errors. |
There was a problem hiding this comment.
Would that be right here?
| // Mark this path as external to avoid messy unwanted resolve errors. | |
| // Mark this module as external to avoid messy unwanted resolve errors. |
There was a problem hiding this comment.
Yes that's right, although this also didn't change in this PR. I can change it though.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
vicb
left a comment
vicb
left a comment
There was a problem hiding this comment.
LGTM high level, I don't know this code base enough to feel confident to approve
| }, | ||
| plugins: [ | ||
| // In Vite 8, `require` calls are not automatically replaced when the format is ESM and `platform` is `neutral` | ||
| // @ts-expect-error: added in Vite 8 |
There was a problem hiding this comment.
Does this not mean this would error in Vite before v8?
There was a problem hiding this comment.
Although I see this is just moved code, so I guess it works already.
There was a problem hiding this comment.
This doesn't execute pre-v8 because isRolldown will be false.
| }, | ||
| plugins: [ | ||
| // In Vite 8, `require` calls are not automatically replaced when the format is ESM and `platform` is `neutral` | ||
| // @ts-expect-error: added in Vite 8 |
There was a problem hiding this comment.
Although I see this is just moved code, so I guess it works already.
4 participants
Fixes #12194.
Avoid collecting
nodejs_compatwarnings during dependency optimization.Previously, a custom plugin was provided during dependency optimization to collect warnings when Node.js built-ins were imported and the
nodejs_compatflag was not enabled.Because optimized dependencies are cached, the warning was only displayed when dependencies changed.
Additionally, it sometimes included false positives from dependencies that were no longer used.
We now always externalize Node.js built-ins during dependency optimization and collect the warnings at runtime.
This is more consistent with how warnings are collected for direct imports of Node.js built-ins.
A picture of a cute animal (not mandatory, but encouraged)