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.

Sign up for GitHub

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

Jump to bottom

fix: update SSR externals only when SSR is enabled (fix #6478) #6492

Merged
merged 2 commits into from Jan 26, 2022
Merged

fix: update SSR externals only when SSR is enabled (fix #6478) #6492

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d388685
fix: update SSR externals only when SSR is enabled
vursen Jan 13, 2022
23a88e6
refactor: apply code review suggestions
vursen Jan 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 6 additions & 4 deletions packages/vite/src/node/optimizer/registerMissing.ts
View file
Open in desktop
Expand Up @@ -54,10 +54,12 @@ export function createMissingImporterRegisterFn(
knownOptimized = newData!.optimized

// update ssr externals
server._ssrExternals = resolveSSRExternal(
server.config,
Object.keys(knownOptimized)
)
if (server.config.ssr) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Suggested change
if (server.config.ssr) {
if (ssr) {

Copy link
Contributor Author

@vursen vursen Jan 25, 2022 •
edited

Choose a reason for hiding this comment

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

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:

vite/packages/vite/src/node/ssr/ssrModuleLoader.ts

Line 222 in e6495f0

const resolved = tryNodeResolve(id, importer, options, false)

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.

Copy link
Member

Choose a reason for hiding this comment

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

@aleclarson could you help us checking this one?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, updated this line. Please, take a look.

server._ssrExternals = resolveSSRExternal(
server.config,
Object.keys(knownOptimized)
)
}

logger.info(colors.green(`✨ dependencies updated, reloading page...`), {
timestamp: true
Expand Down