-
Notifications
You must be signed in to change notification settings - Fork 1.2k
[vite-plugin] use the new resolve.builtins vite config to specify the cloudflare builtins
#8104
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| import assert from "node:assert"; | ||
| import * as fs from "node:fs"; | ||
| import * as fsp from "node:fs/promises"; | ||
| import { builtinModules } from "node:module"; | ||
| import * as path from "node:path"; | ||
| import { fileURLToPath } from "node:url"; | ||
| import { Log, LogLevel, Response as MiniflareResponse } from "miniflare"; | ||
|
|
@@ -25,6 +26,10 @@ import type { | |
| import type { MiniflareOptions, SharedOptions, WorkerOptions } from "miniflare"; | ||
| import type { FetchFunctionOptions } from "vite/module-runner"; | ||
|
|
||
| const nodeBuiltInModules = new Set( | ||
| builtinModules.concat(builtinModules.map((m) => `node:${m}`)) | ||
| ); | ||
|
|
||
| type PersistOptions = Pick< | ||
| SharedOptions, | ||
| | "cachePersist" | ||
|
|
@@ -328,22 +333,16 @@ export function getDevMiniflareOptions( | |
| const [moduleId] = invokePayloadData.data; | ||
| const moduleRE = new RegExp(MODULE_PATTERN); | ||
|
|
||
| // Externalize Worker modules (CompiledWasm, Text, Data) | ||
| if (moduleRE.test(moduleId)) { | ||
| const result = { | ||
| externalize: moduleId, | ||
| type: "module", | ||
| } satisfies vite.FetchResult; | ||
| const shouldExternalize = | ||
| // Worker modules (CompiledWasm, Text, Data) | ||
| moduleRE.test(moduleId) || | ||
| // Node.js builtin node modules (they will be resolved to unenv aliases) | ||
| nodeBuiltInModules.has(moduleId); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Know we've discussed it before but it still doesn't really make sense to me that we have to externalize things again here. Agree this is correct for now though.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I 100% totally agree 👍 I'll go and investigate this 👍 (but yeah for this PR I would keep it as is since this is also effects the other modules) |
||
|
|
||
| return MiniflareResponse.json({ result }); | ||
| } | ||
|
|
||
| // For some reason we need this here for cloudflare built-ins (e.g. `cloudflare:workers`) but not for node built-ins (e.g. `node:path`) | ||
| // See https://github.com/flarelabs-net/vite-plugin-cloudflare/issues/46 | ||
| if (moduleId.startsWith("cloudflare:")) { | ||
| if (shouldExternalize) { | ||
| const result = { | ||
| externalize: moduleId, | ||
| type: "builtin", | ||
| type: "module", | ||
| } satisfies vite.FetchResult; | ||
|
|
||
| return MiniflareResponse.json({ result }); | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.