-
-
Notifications
You must be signed in to change notification settings - Fork 7.4k
feat(ssr): resolve externalized packages with resolve.externalConditions and add module-sync to default external condition
#20409
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.
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
Changes from all commits
3706eee
87d34b9
1e2fc8f
26b1b82
6d38029
7ad1b12
e7c3d47
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,14 +5,16 @@ import fileEntry from '@vitejs/test-entries/file' | |
| // has `exports` key, should resolve to pkg-exports/entry | ||
| import pkgExportsEntry from '@vitejs/test-resolve-pkg-exports/entry' | ||
| import deepFoo from '@vitejs/test-deep-import/foo' | ||
| import deepBar from '@vitejs/test-deep-import/bar' | ||
| // import deepBar from '@vitejs/test-deep-import/bar' | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I commented out this test case as it wasn't working. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does that mean this could be a breaking change if someone relied on the non-standard behavior before? I'm ok with trying this out in a minor, but I think if we're not going to support this case here we can directly remove the test for it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, this code was not working without the change in this PR. |
||
| import moduleSync from '@vitejs/test-module-sync' | ||
| import { used } from './util' | ||
|
|
||
| export default ` | ||
| entries/dir: ${dirEntry} | ||
| entries/file: ${fileEntry} | ||
| pkg-exports/entry: ${pkgExportsEntry} | ||
| deep-import/foo: ${deepFoo} | ||
| deep-import/bar: ${deepBar} | ||
| ${/* `deep-import/bar: ${deepBar}` */ ''} | ||
| module-sync: ${moduleSync} | ||
| util: ${used(['[success]'])} | ||
| ` | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| export default 'module-sync' |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| { | ||
| "name": "@vitejs/test-module-sync", | ||
| "type": "module", | ||
| "private": true, | ||
| "version": "0.0.0", | ||
| "exports": { | ||
| ".": { | ||
| "module-sync": "./index.js" | ||
| } | ||
| } | ||
| } |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a bug fix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
If it is determined as external here,
vite/packages/vite/src/node/plugins/resolve.ts
Lines 348 to 352 in e7c3d47
the resolution here should use
externalConditions.vite/packages/vite/src/node/plugins/resolve.ts
Line 383 in e7c3d47
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, it's only for build? Might not be relevant to this PR specifically but curious what's the idea for
options.isBuildcheck? Is this because Vite crawl imports by ourselves by import analysis resolve (and not resolve for external) during dev, but this is specifically for rollup?If we have
externalizeflag inresolveExportsOrImports, then technically can we also pass this flag instead of swapping offoptions.conditionsforfetchModulenode resolve?vite/packages/vite/src/node/ssr/fetchModule.ts
Lines 51 to 54 in 8033e5b
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is only for build. The purpose is to run this part of code:
vite/packages/vite/src/node/plugins/resolve.ts
Lines 771 to 787 in e7c3d47
In dev, this specifier rewrite is not needed because
fetchModulehandles that.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, we cannot do that. Because
tryNodeResolvewill return a non-absolute id whenexternalizeistrue(e.g.react/jsx-runtime).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the reference 👍 As I understand, the latter part of
processResultwhere it checksresolved.idis fairly an edge case, so previously usingconditionsinstead ofexternalConditionswouldn't have made much difference.I'm still digesting the history of that logic, but will follow up on that on my own.