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.

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

Building with yarn pnp (yarn 2+) ignores the 'browser' element of the package.json #6693

Closed
7 tasks done
francisu opened this issue Jan 31, 2022 · 6 comments
Closed
7 tasks done
Labels
duplicate This issue or pull request already exists pending triage

Comments

@francisu
Copy link

francisu commented Jan 31, 2022

Describe the bug

The title pretty much says it. Unfortunately I don't have the time to provide a reproduction. I can however provide a fix (see below) that I have tested in my environment.

The problem is with yarn pnp the importer ends up having a /* appended to it in some cases (I don't fully understand why). This causes the check of the idToPkgMap in the resolver to fail since it misses, so the browser-specific resolution from the package.json is never found. Solution is to add an entry with the /* to the map in the yarn pnp case.

This shows up clearly when using the aws-sdk for example in a browser app (it will fail at runtime trying to reference a nodejs module).

Index: packages/vite/src/node/plugins/resolve.ts
<+>UTF-8
===================================================================
diff --git a/packages/vite/src/node/plugins/resolve.ts b/packages/vite/src/node/plugins/resolve.ts
--- a/packages/vite/src/node/plugins/resolve.ts	(revision a4895ed9d71531c71243ffcb3aab465ecd5694a0)
+++ b/packages/vite/src/node/plugins/resolve.ts	(date 1643617573205)
@@ -10,6 +10,7 @@
   OPTIMIZABLE_ENTRY_RE
 } from '../constants'
 import {
+  isRunningWithYarnPnp,
   isBuiltin,
   bareImportRE,
   createDebugger,
@@ -557,6 +558,10 @@
 
   // link id to pkg for browser field mapping check
   idToPkgMap.set(resolved, pkg)
+  if (isRunningWithYarnPnp) {
+    // Corresponds to yarn pnp resolve in esBuildDepPlugin
+    idToPkgMap.set(path.dirname(resolved) + '/*', pkg)
+  }
   if (isBuild) {
     // Resolve package side effects for build so that rollup can better
     // perform tree-shaking

Reproduction

none

System Info

System:
    OS: macOS 12.1
    CPU: (12) x64 Intel(R) Core(TM) i7-8850H CPU @ 2.60GHz
    Memory: 19.67 MB / 16.00 GB
    Shell: 5.8 - /bin/zsh
  Binaries:
    Node: 14.17.3 - ~/.nvm/versions/node/v14.17.3/bin/node
    Yarn: 3.1.1 - /usr/local/bin/yarn
    npm: 8.1.0 - ~/.nvm/versions/node/v14.17.3/bin/npm
    Watchman: 4.9.0 - /usr/local/bin/watchman

Used Package Manager

yarn

Logs

No response

Validations

@bluwy
Copy link
Member

bluwy commented Jan 31, 2022

Is this the same as #1979?

@francisu
Copy link
Author

Is this the same as #1979?

Sure is. I can have a look at the use of https://www.npmjs.com/package/@yarnpkg/esbuild-plugin-pnp to resolve this and look at the other problem they hit if that would be helpful. OTOH my fix proposal might be fairly robust for now given that it's pretty straightforward. Like @swandir I'm also curious about the point of the "*" thing.

@swandir
Copy link
Contributor

swandir commented Jan 31, 2022

I've tried fixing this with @yarnpkg/esbuild-plugin-pnp and adding the /* entry in the map.
And though it works for some case, still fails for other. See my comment here: #1979 (comment)

But it turns out the root of the problem is different: resolveDir parameter is passed not just for entry points, but for any module.

I've addressed it along with the other problem from my comment in this PR: #6493

Though we still are to see what maintainers will say about these changes 🙂

@francisu
Copy link
Author

Though we still are to see what maintainers will say about these changes 🙂

Looks like solid work. I hope they will take this fix this quickly as having yarn 2+ broken with AWS for example will slow adoption.

Closing this one since it's a dup.

@Niputi Niputi added the duplicate This issue or pull request already exists label Jan 31, 2022
@swandir
Copy link
Contributor

swandir commented Jan 31, 2022

As a temporary workaround you may apply commits from the PR on top of a Vite version you need, pack a tarball and use it via file: protocol (example)

@francisu
Copy link
Author

francisu commented Feb 1, 2022

As a temporary workaround you may apply commits from the PR on top of a Vite version you need, pack a tarball and use it via file: protocol (example)

I have done that and confirm that your PR fixes my issue, AWS works correctly with yarn pnp in the browser. Thanks for the work on this and I hope they merge it soon!

@github-actions github-actions bot locked and limited conversation to collaborators Feb 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
duplicate This issue or pull request already exists pending triage
Projects
None yet
Development

No branches or pull requests

4 participants