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

fix(optimizer): browser mapping for yarn pnp #6493

Merged
merged 8 commits into from
Jun 16, 2022

Conversation

swandir
Copy link
Contributor

@swandir swandir commented Jan 13, 2022

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:

// 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:

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.

@Niputi Niputi changed the title fix(optimizer): browser mapping for yarn pnp (fix #1979) fix(optimizer): browser mapping for yarn pnp Jan 13, 2022
@Niputi Niputi added the p3-minor-bug An edge case that only affects very specific usage (priority) label Jan 13, 2022
@swandir
Copy link
Contributor Author

swandir commented Jan 27, 2022

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

@francisu
Copy link

francisu commented Feb 2, 2022

I have confirmed that this PR addresses #6693.

@Niputi Niputi requested a review from patak-dev February 2, 2022 07:09
@Niputi Niputi added this to the 2.9 milestone Feb 5, 2022
@swandir swandir requested a review from bluwy February 10, 2022 14:28
Comment on lines +270 to +271
// pass along resolveDir for entries
namespace === 'dep' ? resolveDir : undefined
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@swandir
Copy link
Contributor Author

swandir commented Jun 15, 2022

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?

@patak-dev patak-dev merged commit c1c7af3 into vitejs:main Jun 16, 2022
@swandir swandir deleted the pnp-browser-mapping branch June 16, 2022 08:04
@ferm10n
Copy link

ferm10n commented Jul 11, 2022

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
Copy link
Contributor Author

swandir commented Jul 11, 2022

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
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
No open projects
Status: Todo
Development

Successfully merging this pull request may close these issues.

Vite fails to load browser field from engine.io-client when using yarn berry pnp
6 participants