feat: enable dependencies discovery and pre-bundling in ssr environments #18358

dario-piotrowicz · 2024-10-15T21:09:46Z

Description

This PR adds pre-bundling support for server environments

stackblitz · 2024-10-15T21:09:49Z

Run & review this pull request in StackBlitz Codeflow.

packages/vite/src/node/config.ts

patak-dev · 2024-10-17T10:11:04Z

packages/vite/src/node/external.ts

+    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
+    }


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.

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 🙂👍

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.

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?

dario-piotrowicz · 2024-10-18T09:56:14Z

packages/vite/src/node/optimizer/scan.ts

+    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.')
+    }


I made this change because discovery can start from rollupOptions.input values which can be paths such as:

vite/playground/environment-react-ssr/vite.config.ts

Line 47 in 30975a8

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):

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).

I'll give that a try, thanks! 🙂

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 👀

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?

patak-dev · 2024-10-18T10:55:16Z

/ecosystem-ci run

vite-ecosystem-ci · 2024-10-18T11:09:44Z

📝 Ran ecosystem CI on 30975a8: Open

suite	result	latest scheduled
astro	✅ success	❌ failure
remix	❌ failure	❌ failure
sveltekit	✅ success	❌ failure
vike	❌ failure	❌ failure
vitest	✅ success	❌ failure

✅ 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

patak-dev

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/ssr/fetchModule.ts

sapphi-red · 2024-10-21T06:47:21Z

packages/vite/src/node/external.ts

+    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
+    }


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.

sapphi-red · 2024-10-21T06:56:00Z

packages/vite/src/node/optimizer/scan.ts

+    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.')
+    }


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).

playground/environment-react-ssr/package.json

hi-ogawa · 2024-10-21T07:44:54Z

Awesome work!
I was worrying about what happens when discovery new dep during request handling due to unscanned dynamic import, but it looks like this is not so likely if optimizeDeps.entries are configured well. Even if this happens, it doesn't look like a huge problem either since current module can keep handling requests though it can potentially cause dual modules due to old and new pre-bundled dep. I made a bit contrived example using virtual module to see it in action hi-ogawa@c655057, but I don't think this is a blocker for merging this.

patak-dev · 2024-10-21T08:39:24Z

Awesome work! I was worrying about what happens when discovery new dep during request handling due to unscanned dynamic import, but it looks like this is not so likely if optimizeDeps.entries are configured well. Even if this happens, it doesn't look like a huge problem either since current module can keep handling requests though it can potentially cause dual modules due to old and new pre-bundled dep. I made a bit contrived example using virtual module to see it in action hi-ogawa@c655057, but I don't think this is a blocker for merging this.

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?

hi-ogawa · 2024-10-21T09:04:14Z

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.

patak-dev · 2024-10-21T09:24:25Z

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)

Co-authored-by: 翠 / green <[email protected]>

dario-piotrowicz · 2024-10-21T12:28:05Z

(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 🙇)

dario-piotrowicz force-pushed the dario/ssr-noDiscovery-false branch from 5e9488e to 23d6d9d Compare October 15, 2024 21:16

This comment was marked as off-topic.

Sign in to view

dario-piotrowicz mentioned this pull request Oct 15, 2024

[DO NOT MERGE] pre-bundling example (POC) flarelabs-net/vite-plugin-cloudflare#32

Draft

dario-piotrowicz force-pushed the dario/ssr-noDiscovery-false branch 2 times, most recently from b90f742 to ec8060b Compare October 15, 2024 21:32

This comment was marked as outdated.

Sign in to view

dario-piotrowicz force-pushed the dario/ssr-noDiscovery-false branch from 431d438 to ec7f400 Compare October 17, 2024 09:43

patak-dev reviewed Oct 17, 2024

View reviewed changes

packages/vite/src/node/config.ts Outdated Show resolved Hide resolved

patak-dev reviewed Oct 17, 2024

View reviewed changes

dario-piotrowicz commented Oct 18, 2024

View reviewed changes

dario-piotrowicz marked this pull request as ready for review October 18, 2024 09:56

dario-piotrowicz requested a review from patak-dev October 18, 2024 09:56

patak-dev previously approved these changes Oct 18, 2024

View reviewed changes

patak-dev requested a review from sapphi-red October 18, 2024 11:55

dario-piotrowicz dismissed patak-dev’s stale review via 3c1ca38 October 18, 2024 12:30

sapphi-red reviewed Oct 21, 2024

View reviewed changes

sapphi-red added feat: environment API Vite Environment API p3-significant High priority enhancement (priority) labels Oct 21, 2024

dario-piotrowicz force-pushed the dario/ssr-noDiscovery-false branch 2 times, most recently from 92f90b5 to 3d8d634 Compare October 21, 2024 12:24

feat: enable dependencies discovery and pre-bundling in ssr enviroments

2c7afed

Co-authored-by: 翠 / green <[email protected]>

dario-piotrowicz force-pushed the dario/ssr-noDiscovery-false branch from 3d8d634 to 2c7afed Compare October 21, 2024 12:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: enable dependencies discovery and pre-bundling in ssr environments #18358

feat: enable dependencies discovery and pre-bundling in ssr environments #18358

dario-piotrowicz commented Oct 15, 2024 •

edited

Loading

stackblitz bot commented Oct 15, 2024

This comment was marked as off-topic.

This comment was marked as outdated.

patak-dev Oct 17, 2024

dario-piotrowicz Oct 17, 2024

sapphi-red Oct 21, 2024

dario-piotrowicz Oct 21, 2024

dario-piotrowicz Oct 18, 2024

sapphi-red Oct 21, 2024

dario-piotrowicz Oct 21, 2024

dario-piotrowicz Oct 21, 2024

dario-piotrowicz Oct 21, 2024 •

edited

Loading

patak-dev commented Oct 18, 2024

vite-ecosystem-ci bot commented Oct 18, 2024

patak-dev left a comment

sapphi-red Oct 21, 2024

sapphi-red Oct 21, 2024

hi-ogawa commented Oct 21, 2024

patak-dev commented Oct 21, 2024

hi-ogawa commented Oct 21, 2024 •

edited

Loading

patak-dev commented Oct 21, 2024

dario-piotrowicz commented Oct 21, 2024

feat: enable dependencies discovery and pre-bundling in ssr environments #18358

Are you sure you want to change the base?

feat: enable dependencies discovery and pre-bundling in ssr environments #18358

Conversation

dario-piotrowicz commented Oct 15, 2024 • edited Loading

Description

stackblitz bot commented Oct 15, 2024

This comment was marked as off-topic.

This comment was marked as outdated.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dario-piotrowicz Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

patak-dev commented Oct 18, 2024

vite-ecosystem-ci bot commented Oct 18, 2024

patak-dev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hi-ogawa commented Oct 21, 2024

patak-dev commented Oct 21, 2024

hi-ogawa commented Oct 21, 2024 • edited Loading

patak-dev commented Oct 21, 2024

dario-piotrowicz commented Oct 21, 2024

dario-piotrowicz commented Oct 15, 2024 •

edited

Loading

dario-piotrowicz Oct 21, 2024 •

edited

Loading

hi-ogawa commented Oct 21, 2024 •

edited

Loading