feat: add the `builtins` environment `resolve` #18584

dario-piotrowicz · 2024-11-05T21:23:59Z

Description

The issue this PR is addressing that the current isBuiltin methods seems to be hardcoded for node:

Lines 105 to 109 in ce0eec9

    
           export function isBuiltin(id: string): boolean { 
        
             if (process.versions.deno && id.startsWith(NPM_BUILTIN_NAMESPACE)) return true 
        
             if (process.versions.bun && id.startsWith(BUN_BUILTIN_NAMESPACE)) return true 
        
             return isNodeBuiltin(id) 
        
           }

This doesn't fully seems right to me given the new environment API, since different environments can have different builtins.

For example Cloudflare's workerd runtime has the following builtins:

cloudflare:email
cloudflare:sockets
cloudflare:workers

It can also include the node builtins based on the user's configuration.

So I think that it would be nice if environment authors could declare what builtin modules their environment has.

One final note, without this option it is pretty easy to workaround the hardcoded node isBuiltin method, in dev environments could implement their fetchModule to externalize all their builtins (example) and for build those could be marked as external (example). There are also a few other places where these might need to be set like in optimizeDeps.exclude (example). So this option I'm proposing is probably something not absolutely necessary but just something to avoiding having to specify such modules all over the place (but there might also be other benefits in letting isBuiltin know how to distinguish environment specific builtins 🤷 ).

packages/vite/src/node/optimizer/esbuildDepPlugin.ts

packages/vite/src/node/utils.ts

packages/vite/src/node/config.ts

packages/vite/src/node/external.ts

patak-dev · 2024-11-05T21:46:22Z

I personally think this is a good idea.

sapphi-red · 2024-11-06T03:56:42Z

packages/vite/src/node/optimizer/esbuildDepPlugin.ts

-    if (environment.config.consumer === 'server' && isBuiltin(resolved)) {
+    if (environment.config.resolve.isBuiltin(resolved)) {
      return
    }


I guess this does not work. cloudflare:* will be processed by esbuild and IIRC esbuild does not externalize them automatically and then it throws Could not resolve "cloudflare:*" error. I guess it needs to be return { path: resolved, external: true }.
But I wonder if we should set external: true for anything that was externalized by rollup plugins or resolve.external instead of checking isBuiltin here.

packages/vite/src/node/plugins/resolve.ts

bluwy · 2024-11-06T06:43:55Z

I think I'm ok with this if it helps simplify managing builtins. I was thinking resolve.external could help here, but maybe it's better to reserve that for modules/packages, and it's not quite the perfect fit to use that for optimizeDeps.exclude too I think.

If we do land this, I'd however prefer the option to be an array of strings or regexes instead of a function.

dario-piotrowicz · 2024-11-06T22:59:43Z

If we do land this, I'd however prefer the option to be an array of strings or regexes instead of a function.

I've updated it 🙂

Having this as an array of strings/regexes does feel much more natural 😄 (example diff)

packages/vite/src/node/plugins/resolve.ts

sapphi-red · 2024-12-03T07:43:38Z

packages/vite/src/node/plugins/resolve.ts

+        } else if (
+          currentEnvironmentOptions.consumer === 'client' &&
+          isNodeLikeBuiltin(id)
+        ) {


Suggested change

} else if (

currentEnvironmentOptions.consumer === 'client' &&

isNodeLikeBuiltin(id)

) {

} else if (

currentEnvironmentOptions.consumer === 'server' &&

isNodeLikeBuiltin(id)

) {

if (

options.noExternal === true &&

// if both noExternal and external are true, noExternal will take the higher priority and bundle it.

// only if the id is explicitly listed in external, we will externalize it and skip this error.

(options.external === true || !options.external.includes(id))

) {

let message = `Cannot bundle node built-in module "${id}"`

if (importer) {

message += ` imported from "${path.relative(

process.cwd(),

importer,

)}"`

}

message += `. Consider disabling environments.${this.environment.name}.noExternal or remove the built-in dependency.`

this.error(message)

}

} if (

currentEnvironmentOptions.consumer === 'client' &&

isNodeLikeBuiltin(id)

) {

How about moving the "if options.enableBuiltinNoExternalCheck block" here from above and removing options.enableBuiltinNoExternalCheck && and setting builtin: [] by default for ssr.target: 'webworker'?

By setting builtin: [], I mean updating this line to (consumer === 'server' && !isSsrTargetWebworkerEnvironment.
https://github.com/vitejs/vite/pull/18584/files#diff-11e17761d4ecfee8f8fde15c6d79b7bc0260176396a30dfd8e6f6bbaf5af4745R888

This way if an environment sets builtin and have some node builtin modules not included in builtin, Vite will show a more friendly error (Cannot bundle built-in module) when noExternal is set.

I've made the changes in regards to enableBuiltinNoExternalCheck that you suggested, it sounds sensible to me 🙂👍

Co-authored-by: 翠 / green <[email protected]>

dario-piotrowicz · 2024-12-03T19:29:21Z

I've been trying to think of potential tests to add here but it is a bit tricky 🤔 (since builtins are related to specific environments/runtimes, such as workerd, and I don't think that using a whole environment/runtime just for testing this would be worth it 😕)

if anyone would have any idea that'd be great 🙏 😄

packages/vite/src/node/plugins/resolve.ts

sapphi-red · 2024-12-04T01:53:51Z

I've been trying to think of potential tests to add here but it is a bit tricky 🤔 (since builtins are related to specific environments/runtimes, such as workerd, and I don't think that using a whole environment/runtime just for testing this would be worth it 😕)

if anyone would have any idea that'd be great 🙏 😄

I think there's a following option:

call server.environments.ssr.pluginContainer.resolveId and check the result
example:

vite/packages/vite/src/node/__tests__/resolve.spec.ts

Lines 33 to 35 in d1f1ca0

    
           const resolved = await server.environments.ssr.pluginContainer.resolveId( 
        
             '@vitejs/test-dep-conditions/with-module', 
        
           )

call build and check the output whether it includes import as-is
example:

vite/packages/vite/src/node/__tests__/build.spec.ts

Lines 663 to 682 in d1f1ca0

    
           test('ssr custom', async () => { 
        
             const builder = await createBuilder({ 
        
               root: resolve(__dirname, 'fixtures/dynamic-import'), 
        
               logLevel: 'warn', 
        
               environments: { 
        
                 custom: { 
        
                   build: { 
        
                     ssr: true, 
        
                     rollupOptions: { 
        
                       input: { 
        
                         index: '/entry', 
        
                       }, 
        
                     }, 
        
                   }, 
        
                 }, 
        
               }, 
        
             }) 
        
             const result = await builder.build(builder.environments.custom) 
        
             expect((result as RollupOutput).output[0].code).not.toContain('preload') 
        
           })

But actually we already have miniflare installed at playground/ssr-webworker/vite.config.js. So adding an e2e test there would work too 🙂

packages/vite/src/node/plugins/resolve.ts

dario-piotrowicz · 2024-12-04T11:02:17Z

@sapphi-red thanks a lot for the options for testing this! 🫶

I will try to play with those 🙏

Regarding the miniflare one, right now I would try to avoid using that as I feel that a more generic non-cloudflare specific test would be better, moreover I would not really want to introduce a "miniflare environment" that could confuse people looking at it and thinking that that's the proper way to run use cloudflare/miniflare with the environment API

Co-authored-by: 翠 / green <[email protected]>

sapphi-red

Great! Thank you 😄

patak-dev

sorry 6.1 took a bit more to start than we expected Dario!

dario-piotrowicz · 2025-01-24T11:55:28Z

sorry 6.1 took a bit more to start than we expected Dario!

No, problem! I'm happy to see the PR merged! 🫶 😄

### Description Ported changes from the following PRs: - vitejs/vite#18889 - vitejs/vite#19300 - vitejs/vite#18584, vitejs/vite#19312 and some minor changes. With this PR and #4270, the tests in Vite repo should now pass with native resolve plugin.  --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>

Co-authored-by: 翠 / green <[email protected]>

dario-piotrowicz commented Nov 5, 2024

View reviewed changes

packages/vite/src/node/optimizer/esbuildDepPlugin.ts Outdated Show resolved Hide resolved

dario-piotrowicz commented Nov 5, 2024

View reviewed changes

packages/vite/src/node/utils.ts Outdated Show resolved Hide resolved

patak-dev reviewed Nov 5, 2024

View reviewed changes

packages/vite/src/node/config.ts Outdated Show resolved Hide resolved

patak-dev reviewed Nov 5, 2024

View reviewed changes

packages/vite/src/node/external.ts Outdated Show resolved Hide resolved

dario-piotrowicz mentioned this pull request Nov 5, 2024

use the new resolve.builtins vite config to specify the cloudflare and node builtins flarelabs-net/vite-plugin-cloudflare#52

Closed

dario-piotrowicz changed the title ~~feat: add the new environment config option isBuiltin~~ feat: add the new environment resolve option isBuiltin Nov 6, 2024

sapphi-red reviewed Nov 6, 2024

View reviewed changes

dario-piotrowicz force-pushed the dario/env-isbuiltin branch 2 times, most recently from 572b283 to 4b53f67 Compare November 6, 2024 22:56

dario-piotrowicz changed the title ~~feat: add the new environment resolve option isBuiltin~~ feat: add the builtins environment resolve Nov 8, 2024

feat: add the new environment config option isBuiltin

bc680b8

dario-piotrowicz mentioned this pull request Dec 1, 2024

fix(resolve): don't set builtinModules to external by default #18821

Merged

dario-piotrowicz added 6 commits December 1, 2024 17:06

improve code as suggested in PR

298e0c1

polish code further

63866c9

improve tsdoc comment

4712b5f

revert comment change

98723a4

update resolve.isBuiltin to resolve.builtins

fbdadfc

fix copy-pasta

8c0bc0a

dario-piotrowicz force-pushed the dario/env-isbuiltin branch from e507323 to 8c0bc0a Compare December 1, 2024 17:14

dario-piotrowicz requested review from patak-dev and sapphi-red December 1, 2024 17:14

bluwy added this to the 6.1 milestone Dec 2, 2024

dario-piotrowicz mentioned this pull request Dec 2, 2024

Investigate the fetchModule's cloudflare: catch flarelabs-net/vite-plugin-cloudflare#46

Closed

sapphi-red reviewed Dec 3, 2024

View reviewed changes

dario-piotrowicz and others added 3 commits December 3, 2024 11:16

avoid passing this.environment.config everywhere

0861cb2

Update packages/vite/src/node/plugins/resolve.ts

d2a05d5

Co-authored-by: 翠 / green <[email protected]>

add missing else

e39c3a2

dario-piotrowicz added 2 commits December 3, 2024 12:23

remove incorrect argument

ad579bf

remove enableBuiltinNoExternalCheck as suggested in PR review

d1f1ca0

dario-piotrowicz requested a review from sapphi-red December 3, 2024 15:03

sapphi-red reviewed Dec 4, 2024

View reviewed changes

packages/vite/src/node/plugins/resolve.ts Show resolved Hide resolved

packages/vite/src/node/plugins/resolve.ts Outdated Show resolved Hide resolved

sapphi-red added p3-significant High priority enhancement (priority) feat: environment API Vite Environment API labels Dec 4, 2024

sapphi-red reviewed Dec 4, 2024

View reviewed changes

packages/vite/src/node/plugins/resolve.ts Show resolved Hide resolved

dario-piotrowicz and others added 5 commits December 4, 2024 11:09

Update packages/vite/src/node/plugins/resolve.ts

ae1ed5d

Co-authored-by: 翠 / green <[email protected]>

Update packages/vite/src/node/plugins/resolve.ts

5666f67

Co-authored-by: 翠 / green <[email protected]>

add test for env builtins in dev

8181eeb

remove unused import

1baf5c8

improve tests

0b7d83f

dario-piotrowicz requested a review from sapphi-red December 4, 2024 13:30

dario-piotrowicz added 4 commits December 4, 2024 13:33

remove unnecessary if block

2699eba

add tests for defaults

debad44

improve tests

664273d

update parameter name

09ff292

sapphi-red approved these changes Dec 5, 2024

View reviewed changes

patak-dev approved these changes Jan 23, 2025

View reviewed changes

patak-dev merged commit 2c2d521 into vitejs:main Jan 23, 2025
16 checks passed

This was referenced Jan 29, 2025

fix(resolve): warn if node-like builtin was imported when resolve.builtin is empty #19312

Merged

fix: use nodeLikeBuiltins for ssr.target: 'webworker' without noExternal: true #19313

Merged

sapphi-red mentioned this pull request Apr 23, 2025

feat(plugin/vite-resolve): port changes after Vite 6 rolldown/rolldown#4269

Merged

moonlitusun pushed a commit to moonlitusun/vite that referenced this pull request May 25, 2025

feat: add the builtins environment resolve (vitejs#18584)

8792da4

Co-authored-by: 翠 / green <[email protected]>

	export function isBuiltin(id: string): boolean {
	if (process.versions.deno && id.startsWith(NPM_BUILTIN_NAMESPACE)) return true
	if (process.versions.bun && id.startsWith(BUN_BUILTIN_NAMESPACE)) return true
	return isNodeBuiltin(id)
	}

-        } else if (
-          currentEnvironmentOptions.consumer === 'client' &&
-          isNodeLikeBuiltin(id)
-        ) {
+        } else if (
+          currentEnvironmentOptions.consumer === 'server' &&
+          isNodeLikeBuiltin(id)
+        ) {
+          if (
+            options.noExternal === true &&
+            // if both noExternal and external are true, noExternal will take the higher priority and bundle it.
+            // only if the id is explicitly listed in external, we will externalize it and skip this error.
+            (options.external === true || !options.external.includes(id))
+          ) {
+            let message = `Cannot bundle node built-in module "${id}"`
+            if (importer) {
+              message += ` imported from "${path.relative(
+                process.cwd(),
+                importer,
+              )}"`
+            }
+            message += `. Consider disabling environments.${this.environment.name}.noExternal or remove the built-in dependency.`
+            this.error(message)
+          }
+        } if (
+          currentEnvironmentOptions.consumer === 'client' &&
+          isNodeLikeBuiltin(id)
+        ) {

Uh oh!

feat: add the builtins environment resolve #18584

feat: add the builtins environment resolve #18584

Uh oh!

Conversation

dario-piotrowicz commented Nov 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

patak-dev commented Nov 5, 2024

Uh oh!

sapphi-red Nov 6, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bluwy commented Nov 6, 2024

Uh oh!

dario-piotrowicz commented Nov 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

sapphi-red Dec 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dario-piotrowicz Dec 3, 2024

Choose a reason for hiding this comment

Uh oh!

dario-piotrowicz commented Dec 3, 2024

Uh oh!

Uh oh!

Uh oh!

sapphi-red commented Dec 4, 2024

Uh oh!

Uh oh!

dario-piotrowicz commented Dec 4, 2024

Uh oh!

sapphi-red left a comment

Choose a reason for hiding this comment

Uh oh!

patak-dev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dario-piotrowicz commented Jan 24, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

feat: add the `builtins` environment `resolve` #18584

feat: add the `builtins` environment `resolve` #18584

dario-piotrowicz commented Nov 5, 2024 •

edited

Loading

dario-piotrowicz commented Nov 6, 2024 •

edited

Loading

sapphi-red Dec 3, 2024 •

edited

Loading