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: v6 - Environment API #16471

Merged
merged 276 commits into from
Sep 4, 2024
Merged

feat: v6 - Environment API #16471

merged 276 commits into from
Sep 4, 2024

Conversation

patak-dev
Copy link
Member

@patak-dev patak-dev commented Apr 19, 2024

Description

Note

Check out the Environment API Docs

We're starting a new PR for the Environment API work in progress proposal for v6 to have a clean slate for reviews and discussions now that the implementation and docs are more stable.

This PR merges the work done in:

Refer to the discussions in these PRs for context. If you have general feedback about the APIs, let's use this discussion so we can have proper threads:

If you have comments about the implementation or would like to collaborate a feature, fix, test, or docs, please comment in this PR as an usual review or send a PR against the v6/environment-api branch.

image

Ecosystem Compatibility

These projects are failing in CI because of known reasons.

Framework Reason PR
VitePress using internal server._importGlobMap, which is moved vuejs/vitepress#3706

Copy link

stackblitz bot commented Apr 19, 2024

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

This was referenced Apr 19, 2024
@patak-dev patak-dev added this to the 6.0 milestone Apr 19, 2024
@patak-dev patak-dev added the p3-significant High priority enhancement (priority) label Apr 19, 2024
@patak-dev

This comment was marked as outdated.

@vite-ecosystem-ci

This comment was marked as outdated.

@patak-dev
Copy link
Member Author

After 2866d4f, we have opt-in for build plugins to be shared, matching the way things work during dev.
Individual plugins can set a sharedDuringBuild: true to opt-in, or all plugins can be forced to be shared using builder: { sharedPlugins: true } (this will probably break most plugins right now). There is also a builder: { sharedConfigBuild: true } to directly share the whole config. This will break even more plugins, even ours internally because right now we are all using config.build instead of environment.options.build.

We can deprecate config.build for ResolvedBuildOptions because these are the defaults and shouldn't be used. And later on directly remove config.build after resolving forcing everyone to use environment.options (this is the same we already were discussing to with config.resolve). I think this may be one of the trickiest parts in getting the ecosystem to move. We could always leave an opt-in into backward compatibility when we reach this point.

One interesting problem about shared plugins is that once they are used, the only command that makes sense is vite build --app. Building a single environment could fail because the author of the plugin has the expectation of the plugin processing both client and ssr code for example. This is equivalent to what happens now in SvelteKit and other frameworks where building only ssr doesn't work because you need to build the client first, get the SSR manifest and use it to build the ssr (and they do all this in a single vite build).
So maybe we should not have vite build --environment={name} at all, and have some config opt-in to vite build being vite build --app instead of a CLI option.
Does it ever makes sense to build only a part of an app with Vite? If it is about performance, I think that may need to be cached in a diff way.

@@ -1160,10 +1152,11 @@ const lockfileFormats = [
})
const lockfileNames = lockfileFormats.map((l) => l.name)

function getConfigHash(config: ResolvedConfig, ssr: boolean): string {
function getConfigHash(environment: Environment): string {
Copy link
Collaborator

@hi-ogawa hi-ogawa Sep 3, 2024

Choose a reason for hiding this comment

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

I noticed that getConfigHash became unstable for my use case (react-server environment) hi-ogawa/vite-plugins#634 due to environment.config.resolve.noExternal included as a hash seed, which wasn't the case when config.ssr.noExternal.

I'm using https://github.com/svitejs/vitefu 's crawlFrameworkPkgs to collect noExternal and it's not deterministic. I can normalize it on my side with unique + sort, but maybe we should remove it from JSON.stringify if resolve.noExternal doesn't affect deps optimization? (also what's the case with resolve.external? does it affect deps optimization?)

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should sort it ourselves then? 🤔 We sort other options

Copy link
Member Author

Choose a reason for hiding this comment

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

Interesting. This is SSR only (we need to review if we can use them for the client too). And ya, I think it should not affect the fixed optimized deps we have during SSR 🤔
Maybe we should just remove it as you propose it for now.

@patak-dev
Copy link
Member Author

/ecosystem-ci run

@vite-ecosystem-ci
Copy link

@patak-dev
Copy link
Member Author

Current ecosystem CI state in the Environment API PR. We're all green in main, so we know that these are produced by the new changes:

  • astro: A single CSS test is failing (@sapphi-red's explanation of the issue here
  • remix, vike: expectation to HMR or reload the client when a SSR only module is changed. We consider that this is a bug fix and wasn't intended before. This should be handled on the framework side
  • histoire, vitest: there are PR to fix these that haven't been merged yet

Note: SvelteKit was passing before, it is failing in main now too. It is unrelated to this PR.

@patak-dev
Copy link
Member Author

We discussed in today's Vite team meeting that we should merge this PR so we move further work to main. The changes in this PR are now backward compatible (see ecosystem-ci comment above) and all new APIs are experimental. We can continue refining them during the Vite 6 beta period that we will start now (and even after the stable release, we'll have opportunities for changes if needed until these are stabilized in 6.x).
Thanks everyone for all the work to get this PR to this point 🎉

Copy link
Member

@antfu antfu left a comment

Choose a reason for hiding this comment

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

Let's go! 🔥

@patak-dev patak-dev merged commit 242f550 into main Sep 4, 2024
12 checks passed
@patak-dev patak-dev deleted the v6/environment-api branch September 4, 2024 09:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p3-significant High priority enhancement (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet