-
-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
base: main
Are you sure you want to change the base?
feat: v6 - Environment API #16471
Conversation
Run & review this pull request in StackBlitz Codeflow. |
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 comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
…in the PR and discussions now
Co-authored-by: patak-dev <matias.capeletto@gmail.com>
/ecosystem-ci run |
1 similar comment
/ecosystem-ci run |
📝 Ran ecosystem CI on
✅ analogjs, ladle, laravel, nuxt, quasar, unocss, vite-plugin-pwa, vite-plugin-react, vite-plugin-react-pages, vite-plugin-react-swc, vite-plugin-vue, vite-setup-catalogue |
📝 Ran ecosystem CI on
✅ analogjs, ladle, laravel, nuxt, quasar, unocss, vite-plugin-pwa, vite-plugin-react, vite-plugin-react-pages, vite-plugin-react-swc, vite-plugin-vue, vite-setup-catalogue |
/ecosystem-ci run |
📝 Ran ecosystem CI on
✅ 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 |
…idated on the server
/ecosystem-ci run |
📝 Ran ecosystem CI on
✅ 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, | ||
{ |
There was a problem hiding this comment.
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?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or actually, shouldn't this inherit from the environment's options, some fields are indeed inherited:
https://github.com/vitejs/vite/pull/16471/files/2b3329d4aa0b91a5920527a9712b82ae59becba3#diff-fde6fc19368b69a4726cb38039db6dee6abf54bc69e1484c4f6e6c9d952acd9dR40-R42
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I agree that it needs to be customizable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess here we can only expose these:
mainFields
->externalMainFields
extensions
->externalExtensions
tryEsmOnly
->externalTryEsmOnly
All other fields are related to externalization and not to the environment. Is this ok for you?
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.Ecosystem Compatibility
These projects are failing in CI because of known reasons.
server._importGlobMap
, which is moved