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

mrm007 · 2022-09-05T04:10:30Z

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

vite/packages/vite/src/node/plugins/resolve.ts

Lines 719 to 721 in e7712ff

    
           if (!pkg?.data.exports && path.extname(id) !== resolvedExt) { 
        
             resolvedId += resolvedExt 
        
           }

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                            

> ssr-resolve@0.0.0 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/autosuggest-highlight@3.3.4/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/autosuggest-highlight@3.3.4/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/lodash@4.17.21/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/lodash@4.17.21/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/react-dom@18.2.0_react@18.2.0/node_modules/react-dom/server.node.js +2ms
  vite:resolve-details [node/deep-import] ./server -> /__/vite/node_modules/.pnpm/react-dom@18.2.0_react@18.2.0/node_modules/react-dom/server.node.js +1ms
✓ 1 modules transformed.
dist/main.mjs   0.27 KiB

without fix (main)

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

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

> ssr-resolve@0.0.0 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/autosuggest-highlight@3.3.4/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/autosuggest-highlight@3.3.4/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/lodash@4.17.21/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/lodash@4.17.21/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/react-dom@18.2.0_react@18.2.0/node_modules/react-dom/server.node.js +1ms
  vite:resolve-details [node/deep-import] ./server -> /__/vite/node_modules/.pnpm/react-dom@18.2.0_react@18.2.0/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?

Bug fix
New Feature
Documentation update
Other

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

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

patak-dev

LGTM. Thanks a lot for the fix!

patak-dev · 2022-09-05T21:03:01Z

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 · 2022-09-06T02:10:32Z

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!

patak-dev

Makes sense. Thanks a lot for reworking the test cases and for the clear exposition of your steps on this PR.

playground/ssr-resolve/pkg-exports/index.js

playground/ssr-resolve/vite.config.js

mrm007 · 2022-09-20T03:48:38Z

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

patak-dev · 2022-09-20T08:41:15Z

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

bluwy

Didn't realized a requested review from me. This looks good 👍 I second @ydcjeff's changes too before merging.

benjervis · 2022-10-05T05:18:58Z

@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 · 2022-10-05T07:32:16Z

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

…#9989)

benjervis · 2022-10-05T22:12:28Z

@patak-dev I've opened #10354

…#9989)

mrm007 and others added 3 commits September 5, 2022 00:18

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

94b1a31

test: add tests for resolve plugin fix

a2e57a7

chore: clean up

8ade09d

mrm007 changed the title ~~Fix resolution algorithm when build.ssr is true~~ fix(build): fix resolution algorithm when build.ssr is true Sep 5, 2022

patak-dev previously approved these changes Sep 5, 2022

View reviewed changes

patak-dev added p3-minor-bug An edge case that only affects very specific usage (priority) feat: ssr labels Sep 5, 2022

chore: core review feedback

ebaaaf7

mrm007 dismissed patak-dev’s stale review via ebaaaf7 September 6, 2022 02:09

patak-dev previously approved these changes Sep 6, 2022

View reviewed changes

patak-dev requested a review from bluwy September 6, 2022 20:26

ydcjeff reviewed Sep 7, 2022

View reviewed changes

playground/ssr-resolve/pkg-exports/index.js Show resolved Hide resolved

playground/ssr-resolve/vite.config.js Outdated Show resolved Hide resolved

bluwy reviewed Sep 20, 2022

View reviewed changes

chore: code review feedback

41610f3

mrm007 dismissed patak-dev’s stale review via 41610f3 September 21, 2022 01:51

bluwy approved these changes Sep 21, 2022

View reviewed changes

patak-dev approved these changes Sep 21, 2022

View reviewed changes

patak-dev merged commit 7229251 into vitejs:main Sep 21, 2022

benjervis pushed a commit to benjervis/vite that referenced this pull request Oct 5, 2022

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

b5f8824

…#9989)

benjervis mentioned this pull request Oct 5, 2022

fix(build): fix resolution algorithm when build.ssr is true (#9989) #10354

Closed

antfu pushed a commit that referenced this pull request Oct 6, 2022

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

f20d285

mrm007 mentioned this pull request Oct 11, 2022

Use Vite (again) seek-oss/crackle#47

Merged

cuuc20072008 added a commit to cuuc20072008/vite that referenced this pull request Nov 4, 2022

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

97d5a7a

…#9989)

cuuc20072008 added a commit to cuuc20072008/vite that referenced this pull request Nov 4, 2022

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

e70d0d5

…#9989)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mrm007 commented Sep 5, 2022 •

edited

patak-dev left a comment

patak-dev commented Sep 5, 2022

mrm007 commented Sep 6, 2022 •

edited

patak-dev left a comment

mrm007 commented Sep 20, 2022

patak-dev commented Sep 20, 2022

bluwy left a comment

benjervis commented Oct 5, 2022

patak-dev commented Oct 5, 2022

benjervis commented Oct 5, 2022

	if (!pkg?.data.exports && path.extname(id) !== resolvedExt) {
	resolvedId += resolvedExt
	}

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

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

Conversation

mrm007 commented Sep 5, 2022 • edited

Description

Additional context

What is the purpose of this pull request?

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

patak-dev left a comment

Choose a reason for hiding this comment

patak-dev commented Sep 5, 2022

mrm007 commented Sep 6, 2022 • edited

patak-dev left a comment

Choose a reason for hiding this comment

mrm007 commented Sep 20, 2022

patak-dev commented Sep 20, 2022

bluwy left a comment

Choose a reason for hiding this comment

benjervis commented Oct 5, 2022

patak-dev commented Oct 5, 2022

benjervis commented Oct 5, 2022

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

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

mrm007 commented Sep 5, 2022 •

edited

mrm007 commented Sep 6, 2022 •

edited