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(ssr): skip resolving browser field for SSR build #3039

Merged
merged 2 commits into from
Apr 21, 2021

Conversation

underfin
Copy link
Member

fix: #3036

Description

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.

patak-dev
patak-dev previously approved these changes Apr 18, 2021
@GrygrFlzr GrygrFlzr changed the title fix(ssr): not resolve broswer filed for ssr build fix(ssr): skip resolving browser field for SSR build Apr 18, 2021
GrygrFlzr
GrygrFlzr previously approved these changes Apr 18, 2021
@stupidawesome
Copy link

I believe that this line causes the entry point to become cached, so if the client bundle runs before the SSR bundle, the SSR bundle will re-use the client bundle entry point for a dependency it has seen before, which will always be a browser entry point.

@patak-dev
Copy link
Member

@stupidawesome good catch. @underfin looks like there is a package data cache that may need to be different for ssr.

@stupidawesome
Copy link

stupidawesome commented Apr 20, 2021

I pushed a fix so that the package cache is cleared each time the resolve plugin is resolved. Please check here: stupidawesome@1e9143c

@underfin
Copy link
Member Author

@patak-js @stupidawesome Thanks for your reviewer. Yep, here also need cache for ssr.

@Shinigami92
Copy link
Member

Is it easy to swap tryIndex and ssr parameters? If so, this could be a bit more natural.

@patak-dev patak-dev merged commit 61ea320 into main Apr 21, 2021
@frandiox
Copy link
Contributor

frandiox commented Apr 21, 2021

@patak-js @underfin Hi! I'm a bit late, just saw the link on Discord. I just wanted to ask, would this affect browser-like environments such as Cloudflare Workers? Or is it still configurable?

As a reference, Webpack resolves ['browser', 'module', 'main'], in order, when the target is webworker.
Thanks!

@patak-dev
Copy link
Member

Thanks for the heads up @frandiox, this merged PR would also be affected no? #2996
This is already in v2.2, so that would count as a regression for webworkers. Maybe you could create an issue to properly track this if that is the case

@underfin maybe we should use a "resolveBrowserField" (or better name) instead of "ssr" for the logic that was added to this PR?

@patak-dev
Copy link
Member

This PR is correct, as we can assume that we are in Node when doing ssr. The issue with webworkers is not really a regression since they were depending on a bug of Vite. We discussed that it would be good to add a target: 'webworker' as a feature to properly support running the ssr build on the edge.

@patak-dev patak-dev mentioned this pull request Apr 26, 2021
4 tasks
TobiasMelen pushed a commit to TobiasMelen/vite that referenced this pull request May 3, 2021
speigg added a commit to speigg/vite that referenced this pull request Jul 7, 2021
speigg added a commit to speigg/vite that referenced this pull request Jul 7, 2021
speigg added a commit to speigg/vite that referenced this pull request Jul 7, 2021
Infer socket hostname/port from script location

Fix vitejs#3039
@zhangyuang
Copy link
Contributor

zhangyuang commented Nov 29, 2021

@underfin @patak-js Why there only tryIndex when no exports field 🤔? It will be error in below code

image

// error
import Button from 'vant/lib/button'
// succeed
import Button from 'vant/lib/button/index.js'

@patak-dev
Copy link
Member

@zhangyuang would you open an issue with a minimal repro and linking to this PR so we can properly track this problem?

@zhangyuang zhangyuang mentioned this pull request Nov 29, 2021
9 tasks
@antfu antfu deleted the ssr-broswer-exclude branch December 24, 2021 05:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Isomorphic libraries use wrong entry point when building SSR bundle
7 participants