-
-
Notifications
You must be signed in to change notification settings - Fork 6.3k
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: support resolve.alias
per environment in Environment API
#17583
feat: support resolve.alias
per environment in Environment API
#17583
Conversation
Run & review this pull request in StackBlitz Codeflow. |
There are errors because the formatter wasn't run. It should be automatic when you commit. But you can run it manually if not. We have a lot of playgrounds already, so we should try to avoid new ones as much as possible. Maybe you could try adding the test to the environment-react-ssr playground? Maybe later on we can have a more generic one for these tests. |
packages/vite/src/node/idResolver.ts
Outdated
@@ -40,7 +40,7 @@ export function createIdResolver( | |||
pluginContainer = await createEnvironmentPluginContainer( | |||
environment as Environment, | |||
[ | |||
aliasPlugin({ entries: config.resolve.alias }), // TODO: resolve.alias per environment? | |||
aliasPlugin({ entries: config.resolve.alias }), |
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.
This should be environment.options.resolve.alias
, the config has the default values. We were thinking that we should deprecate these defaults on the ResolvedConfig
and remove them later to avoid these issues.
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 kept this as a fallback and collecting the environment specific resolve.alias
in the plugin, but if it's not needed, I can change this to pass it directly. Makes total sense as this part is creating a plugin container for the environment.
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.
Ah, I see the new alias plugin. I think we don't need it. We can use the option here and keep everything else as is.
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 created the new plugin to work with resolve.alias
out of the box. If you advise against that, then check my latest change. I changed the test to be a unit test and it's only using createIdResolver
, but the consumer needs to handle the environment alias resolution using a custom plugin to make it work, check comment at #17583 (comment).
I only see that the deadlock test is failing, which was failing before my changes too and also locally, but I will re-check the formatter.
I will give it a go, probably I can integrate these tests into that playground. |
Re-run |
Ah, sorry for the noise. Format is ok, nvm that. |
Maybe can we write this as unit test? We have a few tests manually target vite/packages/vite/src/node/ssr/__tests__/ssrLoadModule.spec.ts Lines 66 to 67 in 40f45f6
vite/packages/vite/src/node/__tests__/build.spec.ts Lines 611 to 613 in 40f45f6
I'm not sure if we have a single spec file testing both dev and build, but maybe we could add something like |
f217c53
to
9e4cf4e
Compare
Thanks @hi-ogawa very good suggestion! I implemented the test as a unit test now and yes it's much performant and also produces less noise. @patak-dev I checked your suggested change and it works with one caveat. It seems the consumer needs to add a plugin and use the undocumented let idResolver: ResolveIdFn
return {
name: 'environment-alias-test-plugin',
configResolved(config) {
idResolver = createIdResolver(config, {})
},
async resolveId(id) {
return await idResolver(this.environment, id)
},
} |
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.
Sorry for the delay @lazarv! Thanks for the PR 🙏🏼
Description
This PR implements the suggested solution in #17582 and adds a test playground named
environment-alias
to check that the new alias plugin is using theresolve.alias
specified per environment.