Skip to content
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: separate module graphs per environment #16003

Closed
wants to merge 47 commits into from

Conversation

patak-dev
Copy link
Member

@patak-dev patak-dev commented Feb 21, 2024

Description

We've been discussing a review of Vite's server.moduleGraph API. The idea would be to have separate generic module graphs for each environment instead of a single graph where each module node has mixed properties for the client (browser) and ssr (server) graphs.

Instead of

// single graph with ModuleNode representing a module that 
// could be client only, ssr only, or be part of both graphs
server.moduleGraph 
  ModuleNode { 
    transformResult
    ssrTransformResult
    clientImportedModules
    ssrImportedModules
    ...
  }

This PR proposes:

// a module graph for each environment
server.moduleGraph { browser, server } // later on rsc, or other graphs
  // nodes don't need prefixed properties, and 
  // it is clear to what environment they belong
  ModuleNode {
    transformResult
    importedModules
    ...
  }

Sending this PR as a draft so we can use it as a proof of concept to keep discussing.
Some details:

  • Currently the PR separates the browser and server graph, but doesn't yet allow adding new graphs. @sheremet-va feel free to commit to this PR if you have an idea for how the API/implementation would look for adding a new module graph when a environment is added to the server.
  • We need to pass a environment string to resolveId, load, and transform and not only a ssr boolean because we want to support one graph per environment (even if currently there are only two graphs). We may need to pass environment in other places.
  • server.moduleGraph keeps the same public methods API as before, returning a proxy that tries to be backward compatible with the previous mixed graph. Vitest uses getModuleById and getModulesByFile so these have been tested by passing Vite's CI. The others are not yet tested, and may need more work. I think we don't need to support the non-function members of server.moduleGraph (all the maps).
  • There is one failing test (handle isomorphic module updates) that I skipped for now to show the CI working for the draft. @sheremet-va maybe you could check it out if you have some time.
  • The Runtime API may need to be modified for proper HMR, the SSR graph now have all the properties related to HMR separated.

Some initial open questions:

  • How to add a new graph for each new environment. And how to make they API type safe. I added a getModuleGraph(environment: string) method for now, but it doesn't feel right.
  • Should we deprecate the ssr flag? Is environment !== 'browser' equivalent to it? Or should each environment define if it is ssr or not (currently the flag is used for some branches in core plugins.
  • Should the user be able to configure each environment? If there are three environments, we may need to have entries in the config for each. The browser is configured at the root. The server may be kept as ssr: { ... }. Should we have a rsc: { ... } or edge: { ... }? Each with its own external/noExternal/conditions?
  • Should we remove ssr.target: 'node' | 'webworker' and make webworker be a environment?
  • Is performance affected by having separate module graphs (not counting the backward compatible schemes)

What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Copy link

stackblitz bot commented Feb 21, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@patak-dev patak-dev added feat: hmr p2-to-be-discussed Enhancement under consideration (priority) feat: dev dev server feat: environment API Vite Environment API labels Feb 21, 2024
@vite-ecosystem-ci

This comment was marked as outdated.

@patak-dev
Copy link
Member Author

Nuxt is failing because Property 'idToModuleMap' does not exist on type 'ModuleGraphs'. We could try to support the maps too, but in this case, I think it is easier to ask Nuxt to use .getModuleById() instead. Same with vike with urlToModuleMap and Qwik with fileToModulesMap.

Vitest and Svelte are failing because of ssrTransformResult not being in the ModuleNode. I think I'm missing wrapping the nodes in proxies for some of the backward compatible methods (like in updateModuleInfo)

@vite-ecosystem-ci

This comment was marked as outdated.

@vite-ecosystem-ci

This comment was marked as outdated.

@patak-dev
Copy link
Member Author

Nuxt is now working. It seems there is ton of direct usage for the ModuleGraph maps, so we can't just ask the main frameworks to update. They are also iterating over the maps, so it looks like the maps should be backward compatible too. Here is the commit to add the compat layers for maps

@vite-ecosystem-ci

This comment was marked as outdated.

@vite-ecosystem-ci

This comment was marked as outdated.

@vite-ecosystem-ci

This comment was marked as outdated.

@vite-ecosystem-ci
Copy link

📝 Ran ecosystem CI on 22306e0: Open

suite result latest scheduled
previewjs success success

@patak-dev
Copy link
Member Author

previewjs was failing because they used the ModuleGraph type to refer to server.moduleGraph that now has ModuleGraphs type. I made ModuleGraphs compatible with ModuleGraph, stretching a bit the compat layer (added a runtime: 'mixed' to ModuleGraphs for example) and it seems previewjs is happy now.

VitePress was failing because they are using the internal server._importGlobMap prop and I modified it before to support multiple runtimes. @brc-dd would it be possible to review why VitePress is using this internal prop and maybe creating a feature request in Vite to see if we can replace that with a proper public API?

Qwik and Rakkas are failing due to a puppeteer issue unrelated to this PR.

We aren't that bad now. The CIs that are still failing are: vite-plugin-svelte, SvelteKit, vite-plugin-react, vike, histoire, and astro.

@brc-dd
Copy link
Contributor

brc-dd commented Feb 22, 2024

Regarding VitePress, I am not completely sure, it's Evan's code 😅. But my guess is it's to trigger hmr when any of the files matching the watch pattern is added / removed. Probably we can write our own logic but maybe Evan wanted to reuse stuff (because that code is there since vite 2 and vitepress then was using too many vite internals).

@vite-ecosystem-ci

This comment was marked as outdated.

@patak-dev

This comment was marked as outdated.

@vite-ecosystem-ci

This comment was marked as outdated.

@patak-dev

This comment was marked as outdated.

@vite-ecosystem-ci

This comment was marked as outdated.

@patak-dev

This comment was marked as outdated.

@vite-ecosystem-ci

This comment was marked as outdated.

@patak-dev

This comment was marked as outdated.

@vite-ecosystem-ci

This comment was marked as outdated.

@patak-dev

This comment was marked as outdated.

@vite-ecosystem-ci

This comment was marked as outdated.

@patak-dev
Copy link
Member Author

/ecosystem-ci run

@vite-ecosystem-ci
Copy link

📝 Ran ecosystem CI on 72346f1: Open

suite result latest scheduled
analogjs success failure
astro failure success
nuxt failure success

histoire, ladle, laravel, marko, previewjs, qwik, rakkas, sveltekit, unocss, vike, vite-plugin-pwa, vite-plugin-react, vite-plugin-react-pages, vite-plugin-react-swc, vite-plugin-svelte, vite-plugin-vue, vite-setup-catalogue, vitepress, vitest

@patak-dev
Copy link
Member Author

This PR is a good reference to see that all downstream projects in ecosystem-ci were passing except for Astro and Nuxt with separate module graphs. Let's close this one as there is no much value to have it open, the latest work is at #16471

@patak-dev patak-dev closed this Jul 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: dev dev server feat: environment API Vite Environment API feat: hmr p2-to-be-discussed Enhancement under consideration (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants