Skip to content

Conversation

@hi-ogawa
Copy link
Contributor

@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.

@bolt-new-by-stackblitz
Copy link

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
Contributor 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
@hi-ogawa hi-ogawa deleted the fix-resolve-cache-external-conditions branch October 15, 2024 02:22
moonlitusun pushed a commit to moonlitusun/vite that referenced this pull request May 25, 2025
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