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 120 commits into
base: main
Choose a base branch
from
Draft

feat: v6 - Environment API #16471

wants to merge 120 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.

@antfu
Copy link
Member

antfu commented May 24, 2024

/ecosystem-ci run

1 similar comment
@antfu
Copy link
Member

antfu commented May 24, 2024

/ecosystem-ci run

@vite-ecosystem-ci
Copy link

📝 Ran ecosystem CI on 909fe28: 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
vite-plugin-svelte failure success
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-vue, vite-setup-catalogue

@vite-ecosystem-ci
Copy link

📝 Ran ecosystem CI on 909fe28: 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
vite-plugin-svelte failure success
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-vue, vite-setup-catalogue

@sheremet-va
Copy link
Member

/ecosystem-ci run

@vite-ecosystem-ci
Copy link

📝 Ran ecosystem CI on 611ce5c: 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

@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

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.

Copy link
Member

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?

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

8 participants