feat: add the builtins environment resolve

[dario-piotrowicz]

Description

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

vite/packages/vite/src/node/utils.ts

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 🤷 ).

[patak-dev]

I personally think this is a good idea.

[sapphi-red]

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.

[bluwy]

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]

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)

[sapphi-red]

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.

[dario-piotrowicz]

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

[dario-piotrowicz]

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 🙏 😄

[sapphi-red]

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 🙂

[dario-piotrowicz]

@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

[sapphi-red]

Great! Thank you 😄