-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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 to consider conditions
and more
#18302
Conversation
|
I created a bench script with Negative perf impact on cpuprofile viz |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should move forward with this fix for now. In the future, we could explore moving the resolve cache to the environment instance and pass it to the resolve plugin options.
Maybe a simpler approach to split the cache for environment is to split I was testing css |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested removing the whole resolvedCache but it seems that degrades perf by ~10%. It seems we need to keep this.
I also tested this branch with #16631. The issue is now fixed and server.ssrLoadModule
resolves to dist/emotion-react.cjs.mjs
even after server.pluginContainer.resolveId
is called.
I was thinking we can instantiate a dedicated vite/packages/vite/src/node/baseEnvironment.ts Lines 67 to 80 in 25868f2
|
I made |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As proposed by @hi-ogawa in the linked PR, we could move forward with this one to get the fix in, and then rework in a future PR to optimize it. @sapphi-red I'll let you merge it if you are ok with this
Description
resolve.conditions
not working reliably on custom environment #18222pluginContainer.resolveId(..., { ssr: true })
works differently fromssrLoadModule
resolution for"module"
entry/exports #16631 tooI simply expanded the cache key of "resolved cache", but I'm not sure if just concatenating everything is the best way in terms of performance.
todo