-
-
Notifications
You must be signed in to change notification settings - Fork 6.1k
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
feat: enable dependencies discovery and pre-bundling in ssr environments #18358
base: main
Are you sure you want to change the base?
feat: enable dependencies discovery and pre-bundling in ssr environments #18358
Conversation
Run & review this pull request in StackBlitz Codeflow. |
5e9488e
to
23d6d9d
Compare
This comment was marked as off-topic.
This comment was marked as off-topic.
b90f742
to
ec8060b
Compare
This comment was marked as outdated.
This comment was marked as outdated.
431d438
to
ec7f400
Compare
packages/vite/src/node/external.ts
Outdated
if ( | ||
!environment.config.dev.optimizeDeps.noDiscovery && | ||
!environment.config.dev.optimizeDeps.exclude?.includes(id) | ||
) { | ||
// If there is server side pre-bundling and the module is not | ||
// in the `exclude` config then it is not external | ||
return false | ||
} |
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.
This one is interesting. We were trying to avoid conflating optimizeDeps.include/exclude with external/noExternal in the past but we will need this check for noDiscovery
to make sense. This condition should be move to line 140 though.
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.
I see, I understand trying not to conflate them, although it does feel like these concepts start to overlap here 🤔
I hope this is not a huge concern but yeah it does feel like this check is needed since we do not want prebundled modules to be run through runExternalModule
In any case I've moved the check 🙂👍
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.
If a dependency is marked as external, I guess we should not optimized it even if it is not excluded from optimizeDeps. I mean dropping this whole if block so that only no-externalized files would be pre-bundled.
Otherwise, for the following dependency graph:
- app -> pkg-a (external: true, not excluded) -> pkg-b
- app -> pkg-c (external: true, manually excluded) -> pkg-b
pkg-a will be prebundled with pkg-b, and pkg-a will use the bundled pkg-b. But pkg-c will use the non-bundled pkg-b because pkg-c uses a normal import
and Vite cannot intercept that. This means there will be a problem similar to dual package hazard.
I think this is difficult to understand by users.
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.
pkg-a will be prebundled with pkg-b, and pkg-a will use the bundled pkg-b. But pkg-c will use the non-bundled pkg-b because pkg-c uses a normal import and Vite cannot intercept that. This means there will be a problem similar to dual package hazard.
Yes I understand this issue, although I imagine that you can always end up in similar situations even today (without environments) if you have multiple packages with the same dependency and exclude one of them, no?
I think this boils down to trusting users to provide correct/sensible configs, if someone does try to use more advanced features such as optimizeDeps
they should be able to understand the potential risks and implications I think? 🤔
Anyways I don't fully understand your suggestion, dropping this if block means that all the external modules will not be pre-bundled, doesn't it? but that's the whole point of this PR; Allowing environments to get opt-in externals pre-bundling without having to specify all the various dependencies to pre-bundle (since that's something we can do already today).
So I'm not sure what your suggestion is, unless I am missing something?
const resolveRollupInputPath = (p: string) => { | ||
const resolvedPath = path.resolve( | ||
environment.config.root, | ||
p.replace(/^\//, ''), | ||
) | ||
if (fs.existsSync(resolvedPath)) return resolvedPath | ||
for (const extension of environment.config.resolve.extensions) { | ||
const pathWithExtension = `${resolvedPath}${extension}` | ||
if (fs.existsSync(pathWithExtension)) return pathWithExtension | ||
} | ||
throw new Error('invalid rollupOptions.input value.') | ||
} |
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.
I made this change because discovery can start from rollupOptions.input
values which can be paths such as:
index: '/src/entry-server', |
The problem is that these paths can start with /
and may also not include the file extension both things that break the path resolution here
I think this shows a slight issue in the fact that optimizeDep.entries
and rollupOptions.input
can be used in the same way here but they do accept different type of values (not a huge issue but I guess this can lead to confusion)
the leading `/` issue
Just to clarify the issue with the leading /
, if an entry in path.resolve
starts with /
then the previous entries are thrown away (this is an expected behavior as per the nodejs docs):
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.
Rollup resolves inputs in the same way as IDs in normal imports (i.e. resolveId
hooks are called). To do it correctly, I think we need to call environment.pluginContainer.resolveId(path)
.
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.
I'll give that a try, thanks! 🙂
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.
environment.pluginContainer.resolveId
works fine but if I provide a rollupOptions.input
set to an incorrect value that doesn't start with /
or ./
then the process hangs (instead of erroring)
e.g.
if I have
rollupOptions: {
input: {
index: 'zrc/entry-server',
},
},
the vite dev server hangs indefinitely
I'm trying to understand why 👀
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.
Ok I think I figured it out, if a path does not start with /
or ./
then vite:resolve
tries to resolve it as a bare import, meaning that it goes through tryOptimizedResolve
but here we're waiting for the scan to complete, so the process just hangs.
Basically with the setup above with these changes:
I never encounter the second debugger
statement
I've fixed it be updating the path before passing it to environment.pluginContainer.resolveId
to add a leading /
in case the path doesn't start with ./
, ../
nor /
does this sound ok to you @sapphi-red?
/ecosystem-ci run |
📝 Ran ecosystem CI on
✅ analogjs, histoire, ladle, laravel, marko, nuxt, previewjs, quasar, qwik, rakkas, redwoodjs, storybook, unocss, vite-environment-examples, vite-plugin-pwa, vite-plugin-react, vite-plugin-react-swc, vite-plugin-svelte, vite-plugin-vue, vite-setup-catalogue, vitepress, vuepress |
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.
There are less reds because the PR isn't based on main, and there were some changes lately.
Thanks for working this out @dario-piotrowicz! Let's wait for another approval before we merge this. I think it would be useful to move forward to let environments experiment with this
packages/vite/src/node/external.ts
Outdated
if ( | ||
!environment.config.dev.optimizeDeps.noDiscovery && | ||
!environment.config.dev.optimizeDeps.exclude?.includes(id) | ||
) { | ||
// If there is server side pre-bundling and the module is not | ||
// in the `exclude` config then it is not external | ||
return false | ||
} |
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.
If a dependency is marked as external, I guess we should not optimized it even if it is not excluded from optimizeDeps. I mean dropping this whole if block so that only no-externalized files would be pre-bundled.
Otherwise, for the following dependency graph:
- app -> pkg-a (external: true, not excluded) -> pkg-b
- app -> pkg-c (external: true, manually excluded) -> pkg-b
pkg-a will be prebundled with pkg-b, and pkg-a will use the bundled pkg-b. But pkg-c will use the non-bundled pkg-b because pkg-c uses a normal import
and Vite cannot intercept that. This means there will be a problem similar to dual package hazard.
I think this is difficult to understand by users.
const resolveRollupInputPath = (p: string) => { | ||
const resolvedPath = path.resolve( | ||
environment.config.root, | ||
p.replace(/^\//, ''), | ||
) | ||
if (fs.existsSync(resolvedPath)) return resolvedPath | ||
for (const extension of environment.config.resolve.extensions) { | ||
const pathWithExtension = `${resolvedPath}${extension}` | ||
if (fs.existsSync(pathWithExtension)) return pathWithExtension | ||
} | ||
throw new Error('invalid rollupOptions.input value.') | ||
} |
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.
Rollup resolves inputs in the same way as IDs in normal imports (i.e. resolveId
hooks are called). To do it correctly, I think we need to call environment.pluginContainer.resolveId(path)
.
Awesome work! |
Wouldn't this be equivalent to an edit event where a user added a new import in a file that caused a full-reload? This should work or we have a bug, no? |
I think the manual edit case is different and it should work fine since request handling starts from importing a fresh entry module. My demo is to show what happens when there is re-optimization in the middle of request handling. |
Ah, you're right. In the browser, it doesn't matter because the full-reload would end up doing the request again so the modules being transformed can be discarded (we throw an outdated request exception in this case in the vite:optimized-deps plugin). We catch this in the transform middleware and send a 504. I don't know what would be the equivalent when using the server. A framework could catch the exception and issue a full-reload in the client if there isn't one in fly? (cc @sheremet-va, maybe you have some thoughts about this) |
92f90b5
to
3d8d634
Compare
Co-authored-by: 翠 / green <[email protected]>
3d8d634
to
2c7afed
Compare
(PS: I had to rebase because of a merge conflict, so since I was already at it I squashed all my commits, sorry if this causes any inconvenience in reviewing the PR 🙇) |
Description
This PR adds pre-bundling support for server environments