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(vite): conditional exports can't be active when key is browser (#18012) #18016

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

NWYLZW
Copy link

@NWYLZW NWYLZW commented Sep 3, 2024

Description

#18012 (comment)

Based on my debugging results, the handling here seems a bit too tightly coupled with SSR. For other users encountering this issue, you can try setting the ssrTarget parameter to 'webworker'. However, I believe this area should take into account that the user has already actively declared the conditional.

Copy link

stackblitz bot commented Sep 3, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@sapphi-red
Copy link
Member

@NWYLZW Would you elaborate your usecase? Are you testing a library using Vitest for browser on Node without any shims like JSDOM?

I think this is related to #9860.
@sheremet-va Do you have any thoughts about this?

@NWYLZW
Copy link
Author

NWYLZW commented Sep 9, 2024

@NWYLZW Would you elaborate your usecase? Are you testing a library using Vitest for browser on Node without any shims like JSDOM?

I don't need JSDOM here. On the other hand, I feel that using the ssr.target parameter to indirectly influence this is a bit strange. This configuration feels a bit messy; we should respect the user's configuration regarding export conditions.

I think this is related to #9860.

The scenario is somewhat similar to what was mentioned in #9860, but the way it is handled causes node to be activated even when I set the conditional for browser. My export has two implementations based on Node.js and the browser, which leads to the problem.

@NWYLZW
Copy link
Author

NWYLZW commented Sep 10, 2024

Hey, I noticed that the current resolve function has switched to using the options.webCompatible check, and this seems to have a significant impact.

I still firmly believe that if a user explicitly declares the corresponding conditional, we should respect their configuration. They shouldn't have to configure another option for this to work. If needed, I can also add related unit tests.

@NWYLZW
Copy link
Author

NWYLZW commented Sep 10, 2024

There are some issues with the unit tests, but it seems that I did not introduce them.

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.

2 participants