fix(optimizer): browser mapping for yarn pnp

[swandir]

Description

fix: #1979

Currently, internal module re-mapping via the browser field in package.json during dependency pre-bundling is not supported when is used with Yarn in PnP mode.

This fails to re-map for the browser, and the original node-targeting module is bundled instead.

{
  "name": "browser-internal",
  "browser": {
    "./node.js": "./browser.js"
  }
}

This happens due to incorrect importer being passed to resolver in esbuildDepPlugin:

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

Lines 59 to 68 in 6ecf9b0

	// explicit resolveDir - this is passed only during yarn pnp resolve for
	// entries
	if (resolveDir) {
	_importer = normalizePath(path.join(resolveDir, '*'))
	} else {
	// map importer ids to file paths for correct resolution
	_importer = importer in qualified ? qualified[importer] : importer
	}
	const resolver = kind.startsWith('require') ? _resolveRequire : _resolve
	return resolver(id, _importer, undefined, ssr)

Here the comment states that resolveDir is passed for package entries only, however it actually is passed unconditionally in PnP compat onResolve:

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

Lines 210 to 216 in 6ecf9b0

	build.onResolve(
	{ filter: /.*/ },
	async ({ path, importer, kind, resolveDir }) => ({
	// pass along resolveDir for entries
	path: await resolve(path, importer, kind, resolveDir)
	})
	)

As a result, tryResolveBrowserMapping in vite:resolve fails to locate related pkg data, which leads to ignoring internal browser field mappings for a package being pre-bundled.

This PR fixed the problem by passing resolveDir only for the actual entry modules, which are detected by comparing namespace to 'dep'.

Here's a CodeSandbox with failing checks: linl.
And here's one with fixed Vite version from this PR: link.

Additional context

---

What is the purpose of this pull request?

• Bug fix
• New Feature
• Documentation update
• Other

Before submitting the PR, please make sure you do the following

• Read the Contributing Guidelines.
• Read the Pull Request Guidelines and follow the Commit Convention.
• Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
• Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
• Ideally, include relevant tests that fail without this PR but pass with it.

[swandir]

Everything seems to be working now. Could maintainers please take a look? 🤗

[francisu]

I have confirmed that this PR addresses #6693.

[swandir]

browser-external proxy from #8338 fixes build-time crashed, but something has changed elsewhere and this workaround doesn't work anymore 🥲 Will have to look into the changes since 2.9.9

[swandir]

Oh, it does work for external dependencies

https://github.com/swandir/vite-browser-external-pnp/blob/1551d40a3cfb8f59915e43f23a43c87b3682bb7c/index.html#L100-L107

vfile here isn't failing at runtime anymore.

But it does not work for local pre-bundled dependencies

https://github.com/swandir/vite-browser-external-pnp/blob/1551d40a3cfb8f59915e43f23a43c87b3682bb7c/vite.config.js#L3

This module is loaded in the browser:
https://github.com/swandir/vite-browser-external-pnp/blob/1551d40a3cfb8f59915e43f23a43c87b3682bb7c/dep/node.js#L4

despite being disabled

https://github.com/swandir/vite-browser-external-pnp/blob/1551d40a3cfb8f59915e43f23a43c87b3682bb7c/dep/package.json#L4

[swandir]

Okay, I think it's good enough. PnP compatibility issues for external dependencies are solved. Local optimized dependencies with browser mappings are still failing in some cases as I described here. But that's rather edge-casey and can be tackled separately.

@bluwy @patak-dev can you please take a look?

[ferm10n]

Any chance we could get this merged in for vite@2 as well? We'd like to be able to use yarn's pnp with vite without switching to vite@beta since it's not recommended for production.

[swandir]

Any chance we could get this merged in for vite@2 as well? We'd like to be able to use yarn's pnp with vite without switching to vite@beta since it's not recommended for production.

For the time being you can create a patch from this commit. Or pack a tarball from a tag and use it via Yarn's file: protocol.

[patak-dev]

@ferm10n Vite 3 will be released as stable this week.