-
-
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: environment api config options rework #17756
feat: environment api config options rework #17756
Conversation
Run & review this pull request in StackBlitz Codeflow. |
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.
Proxy looks good to me. If environment.config.xxx
would suffice for most of the cases, when would users need to go environment.getTopLevelConfig().xxx
?
We still have many internal APIs that take a
|
Yeah, I was thinking Vite needs to use |
Description
Building on top of:
For reference, before #17753, we had:
environment.config
is the top level config (i.e.environment.config.root
)environment.options
is theResolvedEnvironmentOptions
, equivalent toenvironment.config.environments[environment.name]
(i.e.environment.options.resolve.conditions
)The motivation for #17753 is that
environment.config
being the top level config (the shared config instance that has the default values) is error proneenvironment.config.resolve.conditions
should never be used. We discussed deprecating these defaults fromResolvedConfig
and then removing them but that will take a while. We could make the type ofenvironment.config
more strict even if the default options are in the object, but there are other issues.Having the top level config as
environment.getTopLevelConfig()
brought two things to the spotlight:root
andbase
. It would be good to have a more ergonomic way to access these instead ofenvironment.getTopLevelConfig().root
config.flag
toenvironment.options.flag
This PR leaves
environment.getTopLevelConfig()
for when the shared instance is needed, and removesenvironment.options
in favor ofenvironment.config
that has typeResolvedConfig & ResolvedEnvironmentOptions
(maybe the type could be improved). It is currently implemented as:This solves the two issues above and avoids confusion.
environment.config
always returns the configuration for this environment (it doesn't matter if the options are per-environment or shared). There are no longer issues with users accessing the defaults by mistake.Notes: The PR also changes the
ssr
flag forEnvironmentOptions
introduced at 90185f7 because it collides with thessr
object inResolvedConfig
. This was confusing in that commit already but I couldn't came up with a better name. We discussed with @sheremet-va and settled down on renaming it toconsumer: 'client' | 'server'
for now.