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

Draft
wants to merge 133 commits into
base: main
Choose a base branch
from
Draft

feat: v6 - Environment API #16471

wants to merge 133 commits into from

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.

patak-dev and others added 2 commits April 19, 2024 14:42
Co-authored-by: Vladimir Sheremet <sleuths.slews0s@icloud.com>
Co-authored-by: Hiroshi Ogawa <hi.ogawa.zz@gmail.com>
Co-authored-by: 翠 / green <green@sapphi.red>
Co-authored-by: Jun Shindo <46585162+jay-es@users.noreply.github.com>
Co-authored-by: Greg T. Wallace <greg@gregtwallace.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Caven <cavenasdev@gmail.com>
Co-authored-by: sapphi-red <49056869+sapphi-red@users.noreply.github.com>
Co-authored-by: Matyáš Racek <panstromek@seznam.cz>
Co-authored-by: bluwy <bjornlu.dev@gmail.com>
Co-authored-by: Igor Minar <i@igor.dev>
Co-authored-by: Vladimir <sleuths.slews0s@icloud.com>
Co-authored-by: 翠 / green <green@sapphi.red>
Co-authored-by: Clément <clemvnt@gmail.com>
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

/ecosystem-ci run

@vite-ecosystem-ci
Copy link

📝 Ran ecosystem CI on e91e269: Open

suite result latest scheduled
astro failure success
histoire failure success
marko failure success
previewjs failure failure
qwik failure success
rakkas failure success
remix failure success
sveltekit failure failure
vike failure failure
vitepress failure success
vitest failure failure

analogjs, ladle, laravel, nuxt, quasar, unocss, vite-plugin-pwa, vite-plugin-react, vite-plugin-react-pages, vite-plugin-react-swc, vite-plugin-svelte, vite-plugin-vue, vite-setup-catalogue


const resolved = tryNodeResolve(
url,
importer,
{ ...resolveOptions, tryEsmOnly: true },
false,
{
Copy link

Choose a reason for hiding this comment

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

are these options correct?

this fetchModule is what the DevEnvironment class uses right?

these options seem to be specifically targeting node? (given the .cjs extension and the main field usage)

should this be make more generic, or actually could this be made configurable?
(so that DevEnvironment instances could tweak these options as needed?)

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

These are the same options that were used for ssrLoadModule before. It might be a good idea to provide a way to customize it, but you can also intercept the fetchModule call at any time

Choose a reason for hiding this comment

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

what do you mean by intercepting the call? 🙏

Right now what I am doing is creating a new DevEnvironment instance (source):

const devEnv = new ViteDevEnvironment(name, config, {
  hot,
}) as DevEnvironment;

and then using the instance's fetchModule in order to let vite apply its module resolution (source):

const result: any = await devEnv.fetchModule(...(args as [any, any]));

So that I can feed this result back to the module runner (source):

transport: {
  fetchModule: async (...args) => {
    const response = await env.__viteFetchModule.fetch(
      new Request('http://localhost', {
        method: 'POST',
        body: JSON.stringify(args),
      }),
    );
    const result = response.json();
    return result as any;
  },
},

is there a different way which would allow me to fetch the modules with my own resolve options?

Choose a reason for hiding this comment

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

@sheremet-va me and Igor have been looking more into this and we agree that the default node environment should preserve the ssrLoadModule behavior.

The fetchModule implementation in this class is however generally useful for custom environments, and in fact it's already exposed as part of the Environment API, so custom environments get to benefit from it.

For the custom environments to however truly benefit from this implementation, the environment config needs to be honored as I've pointed out above.

In order to preserve the behavior for the default node environment, we just need to ensure that it's default node environment is configured in a way that preserves the original ssrLoadModule implementation.

Would this work for you or are we missing anything else that complicates the matter? Thank you

Copy link
Member Author

Choose a reason for hiding this comment

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

You can see what createIdResolver does here:

async function resolve(
. It is properly using the environment configuration (things like resolve.conditions, resolve.alias still needs to be worked out but that will also work). It is the alias+resolve plugins, instead of the whole plugin pipeline for the environment so custom resolving added by users won't apply (optimized deps ids also don't play a role here). It is currently implemented using these two plugins to reuse the implementation, but I think it would be better at some point to have the resolve logic as free functions we can call instead of needing a plugins container for two plugins.
Internally, we are using this resolver to implement custom resolution. See for example resolving of CSS preprocessor files, that have their own rules:
return (sassResolve ??= createIdResolver(config, {
. We don't let users hook into these.

We also use it to make esbuild follow Vite alias and resolve rules during dependency optimization, so it matches what Rollup will do during build. See here

// default resolver which prefers ESM
. I think checking this file should be useful for you. We are also dealing with externals, CJS and esm, etc.

Choose a reason for hiding this comment

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

Perfect! I'll need to thoroughly look into those! thanks so much 🙏

Choose a reason for hiding this comment

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

PS: I am not sure why but I can't resolve this thread, if you want please feel free to resolve this thread since the original issue has been resolved and this might just add unnecessary noise to this PR at this point (and I can ping you on discord for any further discussion on the resolver situation 🙂)

Copy link
Member

Choose a reason for hiding this comment

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

Anyways he made a great point in the fact that we really need to have the module resolution applied here be the exact same as the one that will be used for the production build (so that this behaves as close as possible to what gets actually deployed), so:

But Vite leaves external modules as is (so, vue will be left as vue, not /node_modules/vue) in production, there is no Vite resolution during build for SSR 🤔 (if noExternal is used for build)

The workerd does the resolution. You said that it works in production somehow.

Choose a reason for hiding this comment

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

The workerd does the resolution. You said that it works in production somehow.

@sheremet-va not really 😅 I said that workerd doesn't do the resolution and that in production the modules are bundled in and not external. The issue is that they need to be external during development because they are cjs (which the Vite dev server can't handle).

So during build they are resolved by the Vite resolution and during development they unfortunately need to be external, but they should be resolved to, as close as possible, the same modules that would be used during the build time resolution

Copy link

@IgorMinar IgorMinar left a comment

Choose a reason for hiding this comment

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

Finally posting my review of the api-vite-environment.md file. Many of the comments were written a few weeks ago, but I forgot to post them, I hope they didn't bit rot too much.

I'm super excited about this work! I think the docs will need one more thorough pass once the code has settled down more.

docs/guide/api-vite-environment.md Outdated Show resolved Hide resolved
docs/guide/api-vite-environment.md Outdated Show resolved Hide resolved
docs/guide/api-vite-environment.md Outdated Show resolved Hide resolved
docs/guide/api-vite-environment.md Outdated Show resolved Hide resolved
docs/guide/api-vite-environment.md Outdated Show resolved Hide resolved
### Registering new environments using hooks
Plugins can add new environments in the `config` hook:

Choose a reason for hiding this comment

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

This is confusing a bit to me.... the plugin here only "provides a configuration" for the environment and doesn't "add" or "create" an environment.

It's unclear from the text/API what runtime will actually power this newly configured environment. Will it be createNodeEnvironment? Or something else? What do I need to do if I want to create an environment to power a web worker in by browser app, how do I create such environment in my plugin?

This coupling of transform/build configuration to the runtime and module runner is what I struggle with the most with the current design.

I think it's important to enable plugins to create new environments and for users to adjust the build/transform config for all (or specific) environments, but I think it is just as important for the plugin's environment not to be tied to a particular js runtime. I see signs of that desire in the current design, but I can't really see how it would work end to end.

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 think this comment may be outdated. Let me know if there is still doubts about how plugins are created by plugins or configured (for dev and build).

docs/guide/api-vite-environment.md Outdated Show resolved Hide resolved
}
```
- `this.environment` is the module execution environment where a file update is currently being processed.

Choose a reason for hiding this comment

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

is this the only way you can expose the environment? Exposing APIs vai this is hard to document/discover and it's especially confusing when this state is mutable as it is the case of environments. How about including it in the HotContext?

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 alternative to this.environment is having an environment param in every hook. We're using this because this is how vanilla Rollup already exposes the plugin context to hooks, and what we provide at the environment instance are Vite specific extensions to that context. See:


## Using environments in the Vite server

A Vite dev server exposes two environments by default: a Client environment and a SSR environment. The client environment is a browser environment by default, and the module runner is implemented by importing the virtual module `/@vite/client` to client apps. The SSR environment runs in the same Node runtime as the Vite server by default and allows application servers to be used to render requests during dev with full HMR support. We'll discuss later how frameworks and users can change the environment types for the default client and SSR environments, or register new environments (for example to have a separate module graph for RSC).

Choose a reason for hiding this comment

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

I've thought about the "client" name quite a bit and I wonder why not just call it a "browser" environment? Client feels simply too generic, as any environment connecting to the vite dev server is its "client".

Choose a reason for hiding this comment

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

We discussed this in person with @patak-dev and he made an argument that browser might need multiple environments (e.g. for web worker) and there are also other non-browser clients, like electron.

My 2c is that we should name environments by the purpose they serve rather than technology that powers them or generic category like "client" that can be easily misunderstood. So following this logic, the default browser environment could be called browserMain or browserUI to represent the browser main thread or ui envionment.

Copy link
Member Author

Choose a reason for hiding this comment

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

Calling browserMain to tauri doesn't seems to me to convey the purpose. I think the purpose of the browser, tauri, electron, web extension, etc is to be the client of the app. There is another reason for the client and ssr naming in that it has been the way we have always called these two environments (there are options prefixed by ssr and by client in the config currently)


const resolved = tryNodeResolve(
url,
importer,
{ ...resolveOptions, tryEsmOnly: true },
false,
{

Choose a reason for hiding this comment

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

I think what is confusing here is that we use fetchModule to get hold of external modules. I believe usually fetchModule is meant to be called from runInlineModule only, because previously Vite made the assumption that once an import enters an external module the rest of the module resolution and evaluation will happen outside of Vite.

This assumption is valid for node which has access the the fs, but not valid for other runtimes that might need to rely on Vite and fetchModule to get hold of the script content for a given resolved module id.

In the latter case, we do want to honor the resolution configuration of the current environment, so that's why in Dario's remix PoC we patched Vite to do that.

Now we could add externalMainFields, etc but that raises three questions which I can't answer right now:

  1. Why do we need to have a different set of resolution config attributes for external modules? Doesn't mainFields option only apply to resolving external modules nowadays anyway?

  2. What should be the value of these external* options? And who determines that? Are the values just a copy of the original mainFields and friends?

  3. What would happen if we didn't introduce external* options and instead honored the env specific mainFields and friends? What would break?

patak-dev and others added 13 commits June 3, 2024 12:02
Co-authored-by: Igor Minar <i@igor.dev>
Co-authored-by: Igor Minar <i@igor.dev>
Co-authored-by: Igor Minar <i@igor.dev>
Co-authored-by: Igor Minar <i@igor.dev>
Co-authored-by: Igor Minar <i@igor.dev>
Co-authored-by: Igor Minar <i@igor.dev>
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

9 participants