Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Investigate the fetchModule's cloudflare: catch #46

Open
dario-piotrowicz opened this issue Nov 4, 2024 · 3 comments · May be fixed by #52
Open

Investigate the fetchModule's cloudflare: catch #46

dario-piotrowicz opened this issue Nov 4, 2024 · 3 comments · May be fixed by #52
Assignees
@dario-piotrowicz

Comments

@dario-piotrowicz
Copy link
Contributor

In fetchModule, if there is an error we have this catch logic:

vite-plugin-cloudflare/packages/vite-plugin-cloudflare/src/miniflare-options.ts

Lines 230 to 237 in 7adf439

if (moduleId.startsWith('cloudflare:')) {
const result = {
externalize: moduleId,
type: 'module',
} satisfies vite.FetchResult;
return new MiniflareResponse(JSON.stringify(result));
}

that externalizes cloudflare:* modules

Is this really correct?

Shouldn't cloudflare:* modules be externalized already?

vite-plugin-cloudflare/packages/vite-plugin-cloudflare/src/cloudflare-environment.ts

Lines 171 to 175 in 7adf439

external: [
'cloudflare:email',
'cloudflare:sockets',
'cloudflare:workers',
],

(this probably only applies to build 🤔)

I feel like we might be missing some config here, and we probably should not get cloudflare:* in the fetchModule 🤔
(or if this is ok it should be clear as to why we do need this ad-hoc module handling)
@navaru
Copy link

navaru commented Nov 4, 2024

From my understanding, __VITE_FETCH_MODULE__ is a service binding called within miniflare, by a module runner to fetch modules using vite's dev environment.

Simplified:

  1. miniflare: moduleRunner.import(moduleId)
  2. miniflare: await env.__VITE_FETCH_MODULE__.fetch(moduleId, imported, options)
  3. vite: viteDevServer.environments[name].fetchModule(moduleId, imported, options)

Importing a file worker.ts which imports "cloudflare:email" will throw an error and will be caught by the above and marked as external, only available in miniflare.

@dario-piotrowicz dario-piotrowicz self-assigned this Nov 4, 2024
@dario-piotrowicz
Copy link
Contributor Author

I think it all boils down to this ( source ):

// builtins should always be externalized
if (url.startsWith('data:') || isBuiltin(url)) {
  return { externalize: url, type: 'builtin' }
}

when you import directly from userland code this dev environment fetchModule is used, but since isBuiltin is hardcoded we have no way to tell the dev environment that the module is a built in one, unless, as we currently do, we specifically return an externalized module in our fetchModule implementation 😕

I think this might be expected then... 😕

This was referenced Nov 5, 2024
Draft
Merged
petebacondarwin added a commit that referenced this issue Nov 6, 2024
@petebacondarwin
add link to #46 in comment
2d0e9db
petebacondarwin added a commit that referenced this issue Nov 6, 2024
@petebacondarwin
add link to #46 in comment
fa9ee03
petebacondarwin added a commit that referenced this issue Nov 7, 2024
@petebacondarwin
Node compat take 3 (#49)
6a755e6 
* remove unnecessary node-built-ins bits from module resolution playground

* allow playground test variant vite config files to have any sensible extension

* simplify cloudflare external import handling in __VITE_FETCH_MODULE

* Improve module runner error handling

* add node.js compat (minus global) support

* add global support

* add jsdoc

* use a different cache for each playground variant

* only alias Node.js built-ins for environments that need it

* fix formatting

* remove excess runtime.d.ts files

* remove unnecessary eforce pre

* move node-compat stuff out of the main plugin code where possible

* Do not use ? to encode the virtual alias module

* remove unnecesary object spread

* merge aliases

* simplify the code that uses normalized config

* add link to #46 in comment

* add blank line to prettier ignore file

* fix node-compat playground package name

* remove unnecessary console log

* fix casting of caught error

* fix node-compat test scripts

* cleaner way to identify the entry point for injecting globals

* fix entry-point computation

* uncomment fixed test

* simplify node-compat playground typings

* ensure playwright deps are installed for CI jobs

* remove unnecessary prettyignore line

* improve crypto test

* test some aspects of the pg library in CI

* add test of user aliases working with the ones added by our plugin

* Revert "merge aliases"

This reverts commit beef454.

* fix formatting

* fixup! add test of user aliases working with the ones added by our plugin
@dario-piotrowicz
Copy link
Contributor Author

This might be addressed by vitejs/vite#18584

@petebacondarwin petebacondarwin added this to the Initial beta release milestone Dec 4, 2024
@jamesopstad jamesopstad removed this from the Initial beta release milestone Dec 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@dario-piotrowicz dario-piotrowicz

Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@petebacondarwin @navaru @jamesopstad @dario-piotrowicz