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): fix resolve cache key for external conditions #18332

Merged

Conversation

hi-ogawa
Copy link
Collaborator

@hi-ogawa hi-ogawa commented Oct 11, 2024

Description

I realized resolve cache still conflicts for ssr external resolution as it uses conditions: [] and overrideConditions instead internally. To fix this, I'm adding one more to cache key for now.

const resolved = tryNodeResolve(
url,
importer,
{
mainFields: ['main'],
conditions: [],
externalConditions,
external: [],
noExternal: [],
overrideConditions: [
...externalConditions,
'production',
'development',
],

My use case / experiment is to externalize react in both ssr and rsc environments. I found the resolve bug still exists in hi-ogawa/vite-environment-examples#131 when testing pkg-pr-new from main.

Copy link

stackblitz bot commented Oct 11, 2024

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

@hi-ogawa hi-ogawa marked this pull request as ready for review October 11, 2024 10:00
@sapphi-red
Copy link
Member

I guess we should give users a way to remove the default conditions or change the default conditions to be an empty array, now that with the environment api, people would now might want to remove both browser and node conditions. overrideConditions was used to just to avoid breaking changes, and if we have a way to remove the default condition, we no longer need the overrideConditions.

@hi-ogawa
Copy link
Collaborator Author

Yeah, I agree there should be a better way to organize overrideConditions in general. But, should this fix wait for reworking conditions which seems to have a larger scope?

Maybe it would interesting to bring back proper ... conditions #17326 but in a proper breaking way to see how it can simplify things.

Copy link
Member

@patak-dev patak-dev left a comment

Choose a reason for hiding this comment

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

I agree with both here. In other cases, we have the opportunity to change options without a breaking change (with all the options we moved from server to dev), but here I think we are forced to keep resolve.conditions as is. I think we could merge this PR, and then try to replace both options with a better API later on.

@sapphi-red sapphi-red added the p3-minor-bug An edge case that only affects very specific usage (priority) label Oct 15, 2024
@sapphi-red
Copy link
Member

OK, let's merge this PR 👍

@sapphi-red sapphi-red changed the title fix: fix resolve cache key for external conditions fix(resolve): fix resolve cache key for external conditions Oct 15, 2024
@sapphi-red sapphi-red merged commit 93d286c into vitejs:main Oct 15, 2024
16 checks passed
@hi-ogawa hi-ogawa deleted the fix-resolve-cache-external-conditions branch October 15, 2024 02:22
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
None yet
Development

Successfully merging this pull request may close these issues.

3 participants