fix: update SSR externals only when SSR is enabled (fix #6478) #6492
Conversation
|
Correct me if I'm wrong, but there seems to be a complete absence of tests that would make sure SSR externals get updated in the case when Vite optimizes deps after a missing one has been discovered (see I don't have enough time right now to add them from scratch, maybe someone could help me? Or could we consider merging this PR without tests since the fix is rather trivial? |
vursen
changed the title
vursen
changed the title
Shinigami92
added
feat: ssr
p3-minor-bug
|
Hey, are there any updates about this PR? Could it be merged? |
| server.config, | ||
| Object.keys(knownOptimized) | ||
| ) | ||
| if (server.config.ssr) { |
There was a problem hiding this comment.
The ssr param in rerun is what indicates if the server is running in ssr mode. @bluwy could you confirm this, I see that you approved this one
| if (server.config.ssr) { | |
| if (ssr) { |
There was a problem hiding this comment.
Actually, I'm not sure if they are entirely interchangeable and will work exactly the same way. As far as I get this, tryNodeResolve is the only invoker of registerMissing. An important detail about tryNodeResolve is that this function is not always called with ssr === true even when SSR is enabled. See how ssrModuleLoader calls tryNodeResolve as an example:
I don't know 100% how it is intended to work since there are no tests so I just tried to be as close to the original implementation as possible.
Maybe it will be indeed sufficient to call resolveSSRExternal only depending on the ssr param. I would just like to get an additional confirmation before updating this place.
There was a problem hiding this comment.
@aleclarson could you help us checking this one?
There was a problem hiding this comment.
My bad. I think you're right @patak-dev, that slipped off my mind. @vursen regarding your concern, that seems like it should be a true instead, I'm not sure why it's false since it's used by hookNodeResolve. But yes, we do lack tests for this case, and @aleclarson may know more about it.
There was a problem hiding this comment.
The ssr argument is actually undefined in the call above, not false. The false value is for the targetWeb argument. :)
I think we can use ssr instead of server.config.ssr (as suggested by @patak-dev), since the tryNodeResolve call will be removed in #6591.
There was a problem hiding this comment.
OK, updated this line. Please, take a look.
Description
Fixes #6478
What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123).