fix(ssr): use `tryNodeResolve` instead of `resolveFrom` by aleclarson · Pull Request #3951 · vitejs/vite

aleclarson · 2021-06-24T23:26:40Z

Description

This PR makes it so resolve.dedupe and mode are respected in SSR when resolving import specifiers.

Todo

Add tests for SSR module resolution

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.

aleclarson · 2021-06-25T21:46:58Z

I've added a commit that overrides Module._resolveFilename in build mode so that resolve.dedupe is respected. See here: 17eb919

Shinigami92 · 2021-07-03T17:28:48Z

Seems this PR has some errors, pls fix them and revert the draft status after the tests passes

benmccann

Does resolveFrom also need to be updated to tryNodeResolve in ssrExternal? It looks like this PR will need to be rebased, btw.

jplhomer · 2021-09-14T14:30:04Z

Hiya @aleclarson I read somewhere that this PR also adds support for exports for SSR module imports during SSR — is that true? If so, anything I can do to help move this forward?

aleclarson · 2021-09-14T15:56:19Z

Does resolveFrom also need to be updated to tryNodeResolve in ssrExternal?

@benmccann Seems to be the case already

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

Line 63 in 9e06b81

entry = tryNodeResolve(

I read somewhere that this PR also adds support for exports for SSR module imports during SSR — is that true?

@jplhomer Correct

anything I can do to help move this forward?

Evan wants integration tests for SSR module resolution. So you could update an SSR playground (eg: ./packages/playground/ssr-react) with some/all of the possible SSR-specific edge cases around import:

is .js preferred over .mjs
is pkg.main used instead of pkg.module (in package.json)
is module condition in pkg.exports ignored
is node condition in pkg.exports respected
are relative imports handled properly
does resolve.dedupe work as expected in production

I've tested all those cases manually and the implementation is simple enough, so I don't think the juice is worth the squeeze (hence why I haven't written tests; also I use my own fork). Nonetheless, it's what is needed to merge this. :)

benmccann · 2021-11-09T18:22:03Z

@benmccann Seems to be the case already

What about this line?

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

Line 88 in ccc180c

const depRoot = path.dirname(resolveFrom(`${id}/package.json`, root))

It would be nice to remove the export where resolveFrom is declared to ensure that it's not used elsewhere

I also wonder if nestedResolveFrom should be updated to use tryNodeResolve

aleclarson · 2021-11-09T18:25:31Z

What about this line?

Using tryNodeResolve there wouldn't change anything. Do you disagree?

It would be nice to remove the export where resolveFrom is declared to ensure that it's not used elsewhere

I also wonder if nestedResolveFrom should be updated to use tryNodeResolve

I don't see what these changes would fix. Can you elaborate?

benmccann · 2021-11-09T18:45:14Z

Using tryNodeResolve there wouldn't change anything. Do you disagree?

I agree with that. I was somewhat thinking it might save people from accidentally using the wrong one if there's only one implementation and that it would allow us to remove the dependency on resolve, which could be nice. But resolveFrom should be fine for resolving package.json, so am fine to leave it as is for now as well

I don't see what these changes would fix. Can you elaborate?

This one I do think may cause an issue. nestedResolveFrom will use browserify/resolve, which does not support exports in package.json (browserify/resolve#222). That means #3953 will still not be fixed by this PR. E.g. if you have a Svelte library with a transitive dependency which has an exports map, but no main or module declared in its package.json, then it wouldn't be able to resolve it properly

There's some possibility we wouldn't need nestedResolveFrom anymore. @bluwy added it to support including / excluding transitive dependencies in optimizeDeps, but he's also since added an experimental.prebundleSvelteLibraries option to vite-plugin-svelte. If we were able to turn that on for all users and migrate users to it then we wouldn't need to smartly detect which transitive dependencies of Svelte libraries to include / exclude from optimizeDeps. But I don't know if anyone else depends on the feature and it might be nice to have it available for awhile still until we really prove out experimental.prebundleSvelteLibraries and make sure it works in all cases

bluwy · 2021-11-10T03:21:24Z

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

-    basedir = path.dirname(importer)
+    basedir = fs.realpathSync.native(path.dirname(importer))


I'm not sure why we need fs.realpathSync.native for this. Perhaps a comment could help explain it.

I don't remember why I added it xD

Removing for now..

bluwy · 2021-11-10T03:22:43Z

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

-    const pkgPath = resolveFrom(`${id}/package.json`, basedir)
+    const pkgPath = resolve.sync(`${id}/package.json`, {
+      basedir,
+      preserveSymlinks
+    })


I think we can continue using resolveFrom for this as the main branch now supports preserveSymlinks.

bluwy · 2021-11-10T03:48:19Z

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

  } else {
    basedir = root
  }



To further elaborate Ben's comment about nestedResolveFrom, which would be on this line in the main branch. It's currently used to resolve optimiseDeps.include with syntax like my-lib > some-other-lib. It's probably fine to not update nestedResolveFrom as it's currently used to resolve CJS dependencies only, but yeah ideally we want to move away from it too.

Thanks for clarifying. The change to nestedResolveFrom can be done in a future PR 👍

patak-cat · 2021-11-10T21:39:48Z

@benmccann @bluwy would you both review and approve this PR for inclusion? I think we could target the current beta

benmccann

looks good from my end

bluwy

I'm not the most familiar with SSR side of things, but the resolve part looks good to me.

lovasoa · 2021-12-28T01:37:44Z

This pr might be the root cause of sveltejs/kit#3118

It looks like it is choking on https://github.com/mapbox/node-pre-gyp/blob/v0.11.0/lib/pre-binding.js#L20

Niputi · 2021-12-28T01:40:07Z

@lovasoa please open a new issue with a reference to this one and a reproduction instead of commenting

lovasoa · 2021-12-28T01:45:06Z

I already reported this issue to @sveltejs, they'll open an issue here if they find it necessary.

But as I found this suspicious pr, I wanted to mention the sveltejs issue here in case it helps with debugging, or it rings a bell to the original author of the code.

aleclarson added p3-minor-bug An edge case that only affects very specific usage (priority) needs test labels Jun 25, 2021

This comment has been minimized.

Sign in to view

aleclarson force-pushed the feat/ssr-node-resolve branch from 04c06e6 to 17eb919 Compare June 25, 2021 21:45

aleclarson force-pushed the feat/ssr-node-resolve branch 3 times, most recently from d675cf0 to 012dc97 Compare June 27, 2021 20:06

Shinigami92 marked this pull request as draft July 3, 2021 17:28

aleclarson mentioned this pull request Jul 12, 2021

ssrLoadModule does not load nested dependency #3609

Closed

6 tasks

Shinigami92 added the needs rebase label Jul 16, 2021

aleclarson mentioned this pull request Jul 21, 2021

[SSR] Stop resolving externalized dependencies #4340

Closed

aleclarson added a commit to aleclarson/vite that referenced this pull request Jul 30, 2021

fix(ssr): use tryNodeResolve instead of resolveFrom (vitejs#3951)

a6e3078

aleclarson mentioned this pull request Jul 30, 2021

feat(ssr): support ESM-only dependencies #4450

Closed

7 tasks

benmccann reviewed Aug 10, 2021

View reviewed changes

bluwy reviewed Nov 10, 2021

View reviewed changes

This was referenced Nov 10, 2021

Vite doesn't support ESM packages with "exports" field in SSR #3953

Closed

fix: use enhanced-resolve instead of resolve #5593

Closed

fix(ssr): use tryNodeResolve instead of resolveFrom

404f9d3

aleclarson force-pushed the feat/ssr-node-resolve branch from ccc180c to 404f9d3 Compare November 10, 2021 20:30

Niputi removed the needs rebase label Nov 10, 2021

chore: remove unused imports

f1ddda1

aleclarson marked this pull request as ready for review November 10, 2021 20:51

patak-cat requested a review from benmccann November 10, 2021 21:38

benmccann approved these changes Nov 10, 2021

View reviewed changes

bluwy approved these changes Nov 11, 2021

View reviewed changes

patak-cat approved these changes Nov 11, 2021

View reviewed changes

patak-cat merged commit 87c0050 into vitejs:main Nov 11, 2021

bluwy mentioned this pull request Nov 17, 2021

fix(ssr): skip dedupe require in esm #5714

Merged

9 tasks

bluwy mentioned this pull request Dec 28, 2021

importing sqlite3 fails in dev mode sveltejs/kit#3118

Closed

antfu mentioned this pull request Dec 29, 2021

vite:ssr-require-hook helper leaking into production with VitePress #6305

Closed

7 tasks

bluwy mentioned this pull request Jan 9, 2022

Warning 'The following packages did not export their package.json file...' in case of non-svelte packages sveltejs/vite-plugin-svelte#244

Closed

aleclarson deleted the feat/ssr-node-resolve branch February 25, 2022 20:03

bluwy mentioned this pull request Nov 28, 2022

feat(ssr)!: remove dedupe and mode support for CJS #11101

Merged

9 tasks

		basedir = path.dirname(importer)
		basedir = fs.realpathSync.native(path.dirname(importer))

Uh oh!

Conversation

aleclarson commented Jun 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Todo

What is the purpose of this pull request?

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

Uh oh!

This comment has been minimized.

aleclarson commented Jun 25, 2021

Uh oh!

Shinigami92 commented Jul 3, 2021

Uh oh!

benmccann left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jplhomer commented Sep 14, 2021

Uh oh!

aleclarson commented Sep 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benmccann commented Nov 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aleclarson commented Nov 9, 2021

Uh oh!

benmccann commented Nov 9, 2021

Uh oh!

bluwy Nov 10, 2021

Choose a reason for hiding this comment

Uh oh!

aleclarson Nov 10, 2021

Choose a reason for hiding this comment

Uh oh!

bluwy Nov 10, 2021

Choose a reason for hiding this comment

Uh oh!

bluwy Nov 10, 2021

Choose a reason for hiding this comment

Uh oh!

aleclarson Nov 10, 2021

Choose a reason for hiding this comment

Uh oh!

patak-cat commented Nov 10, 2021

Uh oh!

benmccann left a comment

Choose a reason for hiding this comment

Uh oh!

bluwy left a comment

Choose a reason for hiding this comment

Uh oh!

lovasoa commented Dec 28, 2021

Uh oh!

Niputi commented Dec 28, 2021

Uh oh!

lovasoa commented Dec 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

aleclarson commented Jun 24, 2021 •

edited

Loading

benmccann left a comment •

edited

Loading

aleclarson commented Sep 14, 2021 •

edited

Loading

benmccann commented Nov 9, 2021 •

edited

Loading

lovasoa commented Dec 28, 2021 •

edited

Loading