fix(build): fix resolution algorithm when `build.ssr` is true

[mrm007]

Description

This PR fixes the resolution algorithm when build.ssr is true i.e. when tryNodeResolve is called with ssr: true and externalize: true.

I couldn't find any tests covering this edge case, so I added a fixture in playground/ssr-resolve. Related issues/PRs: #8420 #8421.

Additional context

The current resolution produces an invalid path here https://github.com/vitejs/vite/blob/e7712ffb68b24fc6eafb9359548cf92c15a156c1/packages/vite/src/node/plugins/resolve.ts#L719-L721

In the case of autosuggest-highlight, autosuggest-highlight/match should resolve to autosuggest-highlight/match/index.js, not autosuggest-highlight/match + .js which doesn't exist.

with fix

➜ DEBUG='vite:resolve-details' pnpm build                            

> [email protected] build /__/vite/playground/ssr-resolve
> vite build

vite v3.1.0-beta.2 building SSR bundle for production...
  vite:resolve-details [fs] /__/vite/playground/ssr-resolve/main.js -> /__/vite/playground/ssr-resolve/main.js +0ms
transforming (1) main.js
  vite:resolve-details [node/deep-import] ./match -> /__/vite/node_modules/.pnpm/[email protected]/node_modules/autosuggest-highlight/match/index.js +24ms
  vite:resolve-details [processResult] autosuggest-highlight/match -> autosuggest-highlight/match/index.js +0ms
  vite:resolve-details [node/deep-import] ./match -> /__/vite/node_modules/.pnpm/[email protected]/node_modules/autosuggest-highlight/match/index.js +3ms
  vite:resolve-details [processResult] autosuggest-highlight/match -> autosuggest-highlight/match/index.js +0ms
  vite:resolve-details [node/deep-import] ./isInteger -> /__/vite/node_modules/.pnpm/[email protected]/node_modules/lodash/isInteger.js +2ms
  vite:resolve-details [processResult] lodash/isInteger -> lodash/isInteger.js +0ms
  vite:resolve-details [node/deep-import] ./isInteger -> /__/vite/node_modules/.pnpm/[email protected]/node_modules/lodash/isInteger.js +2ms
  vite:resolve-details [processResult] lodash/isInteger -> lodash/isInteger.js +0ms
  vite:resolve-details [node/deep-import] ./server -> /__/vite/node_modules/.pnpm/[email protected][email protected]/node_modules/react-dom/server.node.js +2ms
  vite:resolve-details [node/deep-import] ./server -> /__/vite/node_modules/.pnpm/[email protected][email protected]/node_modules/react-dom/server.node.js +1ms
✓ 1 modules transformed.
dist/main.mjs   0.27 KiB

without fix (main)

:warning: Notice the paths in the lines tagged with [processResult] (autosuggest-highlight/match.js doesn't exist).

➜ DEBUG='vite:resolve-details' pnpm build

> [email protected] build /__/vite/playground/ssr-resolve
> vite build

vite v3.1.0-beta.2 building SSR bundle for production...
  vite:resolve-details [fs] /__/vite/playground/ssr-resolve/main.js -> /__/vite/playground/ssr-resolve/main.js +0ms
transforming (1) main.js
  vite:resolve-details [node/deep-import] ./match -> /__/vite/node_modules/.pnpm/[email protected]/node_modules/autosuggest-highlight/match/index.js +13ms
  vite:resolve-details [processResult] autosuggest-highlight/match -> autosuggest-highlight/match.js +0ms
  vite:resolve-details [node/deep-import] ./match -> /__/vite/node_modules/.pnpm/[email protected]/node_modules/autosuggest-highlight/match/index.js +2ms
  vite:resolve-details [processResult] autosuggest-highlight/match -> autosuggest-highlight/match.js +0ms
  vite:resolve-details [node/deep-import] ./isInteger -> /__/vite/node_modules/.pnpm/[email protected]/node_modules/lodash/isInteger.js +1ms
  vite:resolve-details [processResult] lodash/isInteger -> lodash/isInteger.js +0ms
  vite:resolve-details [node/deep-import] ./isInteger -> /__/vite/node_modules/.pnpm/[email protected]/node_modules/lodash/isInteger.js +0ms
  vite:resolve-details [processResult] lodash/isInteger -> lodash/isInteger.js +0ms
  vite:resolve-details [node/deep-import] ./server -> /__/vite/node_modules/.pnpm/[email protected][email protected]/node_modules/react-dom/server.node.js +1ms
  vite:resolve-details [node/deep-import] ./server -> /__/vite/node_modules/.pnpm/[email protected][email protected]/node_modules/react-dom/server.node.js +1ms
✓ 1 modules transformed.
dist/main.mjs   0.27 KiB

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯ Failed Tests 1 ⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯

 FAIL  playground/ssr-resolve/__tests__/ssr-resolve.spec.ts > correctly resolve entrypoints
Error: [vite-node] Failed to load autosuggest-highlight/match.js
 ❯ async /__/oss/vite/playground-temp/ssr-resolve/dist/main.mjs:1:256
 ❯ async /__/oss/vite/playground/ssr-resolve/__tests__/ssr-resolve.spec.ts:6:31

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[1/1]⎯

---

What is the purpose of this pull request?

• [x] Bug fix
• [ ] New Feature
• [ ] Documentation update
• [ ] Other

Before submitting the PR, please make sure you do the following

• [x] Read the Contributing Guidelines.
• [x] Read the Pull Request Guidelines and follow the Commit Convention.
• [x] Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
• [x] Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
• [x] Ideally, include relevant tests that fail without this PR but pass with it.

[patak-dev]

One improvement we could do before merging is to move the new tests to an existing playground. Our e2e CI is taking quite a bit of time already so we are trying to keep the number of playgrounds at check. Instead of importing real packages in the ssr-resolve you created, we could add new local packages in the ssr-deps repo with these edge cases distilled (check pkg-exports for example).

[mrm007]

Hi @patak-dev, thanks for looking at this!

I had gone that route initially and created packages in ssr-deps, but then a weird thing happened. The issue seems to not be reproducible inside that setup. Whether it's because it uses ssrLoadModule or something else, the tests pass with or without the fix. Here's the branch without the fix and the tests still pass.

Log output from that branch

➜ DEBUG='vite:resolve-details' pnpm -w test-build ssr-deps

> vite-monorepo@ test-build /__/oss/vite
> cross-env VITE_TEST_BUILD=1 vitest run -c vitest.config.e2e.ts "ssr-deps"

✂ ✂ ✂
vite:resolve-details [node/deep-import] ./dir -> /__/oss/vite/node_modules/.pnpm/file+playground+ssr-deps+entries/node_modules/entries/dir/index.js
vite:resolve-details [processResult] entries/dir -> entries/dir.js
✂ ✂ ✂
vite:resolve-details [node/deep-import] ./dir -> /__/oss/vite/node_modules/.pnpm/file+playground+ssr-deps+entries/node_modules/entries/dir/index.js
✂ ✂ ✂

 ✓ playground/ssr-deps/__tests__/ssr-deps.spec.ts  (17 tests) 2172ms

Test Files  1 passed (1)
     Tests  17 passed (17)
  Start at  12:02:55
  Duration  3.51s (transform 449ms, setup 169ms, collect 37ms, tests 2.17s)

So, I've removed the real packages (react, lodash, autosuggest-highlight) and created simple ones with the edge cases distilled. Thanks for the feedback!

[mrm007]

@patak-dev @bluwy can we please get this merged?

[patak-dev]

@mrm007 would you address @ydcjeff comments first? Thanks!

[benjervis]

@patak-dev Do we have any timeline on when this would be released? It doesn't look like it made it into v3.1.4, but we're very keen to be able to move forward with our project.

[patak-dev]

@benjervis would you send a PR cherry-picking this one to the v3.1 branch?

[benjervis]

@patak-dev I've opened https://github.com/vitejs/vite/pull/10354