feat(plugin-api): support async configResolved hooks (fixes #2949) #2951
Conversation
Shinigami92
added
the
p2-nice-to-have
packages/vite/src/node/config.ts
Outdated
| @@ -376,11 +376,11 @@ export async function resolveConfig( | |||
| ) | |||
|
|
|||
| // call configResolved hooks | |||
| userPlugins.forEach((p) => { | |||
| for (const p of userPlugins) { | |||
There was a problem hiding this comment.
Could use Promise.all here, as configResolved should not change the configure object anyway.
There was a problem hiding this comment.
@antfu I think it is better to avoid Promise.all as all other hooks are sequential, and plugins could set up an internal state that will be used by following plugins. For example for presets where you return a plugin that is an array of plugins. I think the performance gains here may not justify the possible subtle issues. What do you think?
There was a problem hiding this comment.
I had Promise.all first but didn't want to change the documented sequential behavior.
There was a problem hiding this comment.
With Promise.all they are still sequential for sync functions and async functions before their first await. I just thought configResolved is designed for plugins to grab the configuration and store it, and that's it. I am not sure if it's ideal to let all the plugins wait for each other for each hook.
There was a problem hiding this comment.
You have a point, and in Rollup, there are a lot of parallel async hooks so we need to choose here.
I think that it was a good idea to make the config hook be sequential async (since the options hook in rollup is also sequential https://rollupjs.org/guide/en/#options). But for this one, I think there is no equivalent hook in rollup so we have to decide. I am fine with this one being parallel, we need to document this in the same way as rollup does in the Plugin API section. There is not mention of hooks being sequential or parallel in our docs at this point.
There was a problem hiding this comment.
the docs do mention sequential, that was the reason i didn't go with Promise.all because i didn't want to change it in case anyone already relies on it. But antfu is right, for sync functions all behaves the same, so i'm indifferent.
My main reasoning for having this hook as async too is that ResolvedConfig passed to configResolved has a lot more information available than UserConfig passed to config. eg isProduction and command.
There was a problem hiding this comment.
You are right, sorry, I only looked for parallel. I think Evan used sequential because it was sync here. @antfu if you think we can go ahead with making this one parallel, I am fine with it. If not, we should request a review from Evan to check.
There was a problem hiding this comment.
Yeah, maybe it's better to have Evan's feedback first.
There was a problem hiding this comment.
I think we could still merge this one as parallel, because later if it is decided to change it to sequential it wouldnt be a breacking change, no? It would be breacking if we changre from sequential to parallel. Or should this be a minor in both cases?
There was a problem hiding this comment.
rebased main and changed to parallel with Promise.all
dominikg
force-pushed
the
feat/async-configResolved
branch
from
06e008e to
fac226c
Compare
Description
see #2949
Additional context
Should docs contain an example?
What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123).