Conversation
6010b2f to
a3c5083
Compare
| @@ -13,7 +16,7 @@ export async function getConfig ( | |||
| ignoreNonAuthSettingsFromLocal?: boolean | |||
| } | |||
| ): Promise<Config> { | |||
There was a problem hiding this comment.
Is the intention of this function to support async updateConfig hooks? At the moment users can provide async implementations. That mostly works because async functions can return promises, and that gets flattened automatically.
If the intention isn't to support async updateConfig, I would check the return type to make sure it's not a promise. If it is intentional, could we await the config.hooks.updateConfig call for correctness?
There was a problem hiding this comment.
ok, added support for async updateConfig
| } | ||
|
|
||
| if (opts.excludeReporter) { | ||
| delete config.reporter // This is a silly workaround because @pnpm/core expects a function as opts.reporter |
There was a problem hiding this comment.
I'm not sure if it's intentional that config.hooks.updateConfig can be a promise returning function, but a heads up that this piece of code won't work as expected if config is a promise.
|
The problem with this way of extending settings is that Renovate doesn't allow the execution of hooks. So, if updateConfig extends overrides, for instance, Renovate will ignore that. Long term, we probably need a static way of extending settings with config dependencies. |
Related comment https://github.com/orgs/pnpm/discussions/9037#discussioncomment-12389019
I am not describing this in the changeset yet as I want to test it more before spreading the word. The goal of this hook is too allow sharing settings via configurational dependencies.