-
Notifications
You must be signed in to change notification settings - Fork 4.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 Vite issues with SolidStart #16052
Conversation
1cc01ac
to
6d0cae7
Compare
6d0cae7
to
81e5f04
Compare
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.
Now that we don't remove any roots anymore, I'm not sure how this will behave over a longer time (memory wise). But the only real issue would be if you create a lot of new CSS files I think anyway so should be fine?
Left a few comments
TEST: 'false', // VERY IMPORTANT OTHERWISE YOU WON'T GET OUTPUT | ||
NODE_ENV: 'development', |
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.
😍
// Note: Removing this during SSR is not safe and will produce | ||
// inconsistent results based on the timing of the removal and | ||
// the order / timing of transforms. | ||
if (!isSSR) { |
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'm not 100% sure why this is safe to do this unconditionally. Is it just because we are never removing any root
s anymore?
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.
We're no longer calling roots.delete(id)
so this check can be removed too, not sure I understand your question correctly tho? 😅
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'm being a dumb dumb, carry on.
8e125d0
to
48871b4
Compare
Yeah I think ultimately this is going to need some sort of LRU cache and some extra bookkeeping but it does seem like it's not safe way to assume a module has been downloaded by trying to look at which servers still have the module loaded :/ |
Fixes #16045
This PR fixes two Vite issues found with SolidStart:
/
) with no pathname. Since we use the path to generate anid
for HTML chunks, this would currently cause a crash. This was reported in @tailwindcss/vitev4.0.1
not workingCannot read properties of undefined (reading 'includes')
#160454.0.0-alpha.22
. After doing some bisecting we found that this is happening as SolidStart has the same module ID in different servers and we were invalidating the root when we shouldn't. After trying to restructure this code so that it only cleans up the root when it is no longer part of any server, we noticed some other compatibility issues with Nuxt and SvelteKit. It seems that the safest bet is to no longer update a root at all during rebuilds in the SSR step. This makesinvalidateAllRoots
a function that only notifiers the servers about a change which is conceptually also less confusing.Test plan