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: environment api #16129

Merged
merged 118 commits into from
Apr 19, 2024
Merged

feat: environment api #16129

merged 118 commits into from
Apr 19, 2024

Conversation

patak-dev
Copy link
Member

@patak-dev patak-dev commented Mar 10, 2024

Description

Important

The Environment API work has been consolidated into #16471

Work in progress implementation of the design outlined at:

Some of the changes that need to happen:

  • Separate module graphs per environments
  • Backward compatible mixed server.moduleGraph
  • Pass Environment { id, type, config } instead of string
  • Move moduleGraph.safeModulePaths to server.safeModulePaths
  • Rework ssrFixStacktrace, maybe it isn't needed anymore
  • Properly implement the new environment aware hotUpdate hook
  • Move transformRequest et al to the environment
  • Move server.hot to the environment
  • Rework Vite Runtime API as ModuleRunner
  • Fine-grained HMR-updates per environment
  • Environment config overrides
  • Backward compat for config changes
  • Environments registration
  • Abstract ssrEnvironment
  • Move getDepsOptimizer to environment
  • workerd environment prototype against this PR

To be implemented in future PRs:

  • Delete ssrError and ssrModule once ssrLoadModule is using the new APIs
  • Explore environment aware vite build
  • Explore RPC layer in core run vs import
  • Dynamic version of the apply option

We can keep improving and discussing the spec in #16089 and later on merge it as the docs PR for this feature. Sending this PR so we have a branch were we can collaborate in the implementation.

The starting point for this PR is a squash of:

#16003 implements the separation of the module graphs for the browser and server environments. Internal plugins in Vite have been refactored to use the independent module graphs. A backward compatibility later has been implemented so server.moduleGraph works for downstream projects in the ecosystem. Only Nuxt and Astro are failing right now in vite-ecosystem-ci for it. We can keep working on fixing backward compatibility in that PR and then merge the fixes here.

So far, I moved the the each EnvironmentModuleGraph inside of ModuleExecutionEnvironment.
Some decisions that diverge from the current spec and we still need to discuss (there or here):

  • There is only a server.environments: Map<string, ModuleExecutionEnvironment> for now. The code is using server.environments.get(environmentId)
  • A ModuleExecutionEnvironment has an id that is unique for the server, and a type. The idea here is to allow for several dev environments of the same type (like workerd) to be used at the same time.
  • We may want to rediscuss nodeEnvironment being called node. If at one point the Vite dev server may be run in a non-node environment, the name may not be the best. Imagine if the landscape evolves into us wanting to use only a standard subset that works in more environments for Vite core. I don't know a better name though, serverEnvironment (as in "the same environment as the server") doesn't work that well either.

What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Copy link

stackblitz bot commented Mar 10, 2024

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

@patak-dev patak-dev added feat: hmr feat: dev dev server feat: environment API Vite Environment API p3-significant High priority enhancement (priority) labels Mar 10, 2024
options?: { ssr?: boolean; environment?: string },
options?: {
ssr?: boolean
environment?: ModuleExecutionEnvironment | BuildEnvironment
Copy link
Member

@sheremet-va sheremet-va Mar 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we guarantee it exists somehow? (non undefined)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that during scan, we didn't pass the moduleGraph. See here

const container = await createPluginContainer(config)

So I didn't pass the environment either. I thought that it may make the scan slower if we need to update info in a module graph that we are going to throw away. But I think it is wrong that we don't pass an environment, more once we will have environment specific config. I'll check what needs to be done here. I think we only need the plugins for resolveId that shouldn't mess with the moduleGraph, so maybe we can create a new temporal browser environment with a new module graph and pass that. I'll check this out, it would be a lot better if there is always an environment defined.

Copy link
Member Author

@patak-dev patak-dev Mar 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's see if there isn't some internalish use cases of these hooks in the ecosystem, but for now, core works with options and environment being required for hooks. See e30b858

I fixed some issues were I missed passing the environment, so even if we revert, it was worth it. We currently have a design issue because an environment needs a resolveId function when it is created, that should be implemented as container.resolveId(id, undefined, { environment }). I'll rework this next so we can simplify the code. Edit: done in 03d3889

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems we'll need to relax the types. See failing test for Vitest. We could leave them as is for a bit more if they help us work with this draft though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I already moved back to optional options and environment. If not, we were forced to have a new GlobalEnvironment that would be used when we don't have a server available (in the plugin containers created during config resolution for example).
Still, this was quite helpful to identify issues. And things looks better after fcebb7d

@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
Copy link

@bluwy bluwy added this to the 5.3 milestone Mar 13, 2024
@patak-dev patak-dev modified the milestones: 5.3, 6.0 Apr 9, 2024
@patak-dev patak-dev changed the base branch from main to v6/environment-api April 19, 2024 12:40
@patak-dev patak-dev marked this pull request as ready for review April 19, 2024 12:41
@patak-dev patak-dev merged commit f684d4c into v6/environment-api Apr 19, 2024
15 of 16 checks passed
@patak-dev patak-dev deleted the feat/environment-api branch April 19, 2024 12:42
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 p3-significant High priority enhancement (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants