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(resolve): improve browser filed substitutions #2701

Merged
merged 2 commits into from
Mar 29, 2021

Conversation

fi3ework
Copy link
Member

Description

Fixes #2598.

Browser filed spec is unmaintained, this issue of esbuild has made a detailed explaination. I think Vite can use the same strategy as esbuild to handle browser field. This PR also refers to test case of esbuild.

Additional context

Didn't mapping all test case from esbuild. I think its test case is a bit redundant.


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.

@anncwb anncwb added p2-nice-to-have Not breaking anything but nice to have (priority) bug labels Mar 26, 2021
Shinigami92
Shinigami92 previously approved these changes Mar 26, 2021
@Timsonrobl
Copy link
Contributor

Timsonrobl commented Mar 26, 2021

Would it work with .mjs extensions or any other user specified extensions from ResolveOptions.extentions?
And AFAIU it could be even not a relative path but any other node-resolvable notation.

@patak-dev
Copy link
Member

Thanks a lot for this PR @fi3ework and the detailed explanation and links! 💯
It helped a lot to understand what was the problem.

@Timsonrobl
Copy link
Contributor

I think @ljharb is right, that ideally browser field should follow node resolution algorithm as close as possible. This would allow developers to substitute or set to ignore native modules like:

browser: {
  'util': false
}

instead of relying on contraptions like that.
Also node resolution algorithm is well known and established and I don't see good reason for deriving separate syntax from current implementations that vary so much.

@patak-dev
Copy link
Member

@Timsonrobl as Evan Wallace said, he was following webpack as much as possible because it is what currently most projects were using. I think we could merge this PR that already fixes several issues at this point.

And we could later have another PR to improve this further if you or someone else would like to add more test cases and review the diffs with other tools.

patak-dev
patak-dev previously approved these changes Mar 27, 2021
Shinigami92
Shinigami92 previously approved these changes Mar 27, 2021
@Shinigami92
Copy link
Member

@fi3ework Could you merge main into your branch? (Or rebase)
That should fix your appveyor test I think

@fi3ework
Copy link
Member Author

@fi3ework Could you merge main into your branch? (Or rebase)
That should fix your appveyor test I think

@Shinigami92 Done, appveyor passed. 😆

@patak-dev patak-dev merged commit cc213c6 into vitejs:main Mar 29, 2021
@fi3ework fi3ework deleted the fix-2598 branch March 29, 2021 18:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2-nice-to-have Not breaking anything but nice to have (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot read property 'custom' of undefined
6 participants