-
-
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
refactor: top level keys for environments (and .environments array) #18439
Conversation
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
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.
Looks nice. The only worry I have is forgetting to sync environments
and $foo
s.
config: ResolvedConfig, | ||
): ResolvedEnvironmentOptions { | ||
return { | ||
name: '$', |
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 think the name
should be passed from the argument.
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.
The name here is a dummy value for defaults. I don't think it would help to pass it from the two places where this function is called. Ideally, it shouldn't be there, but I don't want to make it nullable.
This is the same as in #18426, no? And at least right now environments are only created on server and builder init, so it shouldn't be a problem (except for fixing the issue you saw with the prev |
Ah, yeah it is. I think it's fine. Just that we have to be careful not to forget to call |
Co-authored-by: sapphi-red <49056869+sapphi-red@users.noreply.github.com>
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.
If the majority is onboard, I'm happy with this change too 👍
if (name.startsWith('$')) { | ||
const environmentName = name as `$${string}` |
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.
Maybe we should also disallow naming environments with just $
, but I think we can think about this later.
Closing as one of the main reasons for this exploration is gone after merging: Even without this change, we got a lot of feedback that mixing environments and other config props wasn't worth removing the For later reference, we also asked for feedback about |
Description
Inspired by Nuxt
$production
/$development
top level keys. See environment overrides.Instead of:
this changes the API to use top level keys prefixed by
$
The idea is avoiding the extra nesting as config options become to verbose:
The same environment top level keys are mapped to
ResolvedConfig
,ViteDevServer
,ViteBuilder
resolvedConfig.environments
,viteDevServer.environments
,builder.environments
is now aEnv[]
instead of aRecord<string,Env>
. The object form is no longer needed now that we have access at the top level, and the list simplifies iteration (no need forObject.values()
,Object.keys()
). For example:Built on top of #18426 but moving
.environments
to be an array. It is generally a lot easier to use. A lot ofObject.values()
are removed.Proposal triggered by this discussion #18415 (comment)