feat: add possibility to amend configuration in a plugin#5124
feat: add possibility to amend configuration in a plugin#5124marcoscaceres merged 8 commits intospeced:mainfrom
Conversation
4246477 to
001cf3d
Compare
We have a `preProcess` plugin that needs to store some information for later use when a document is rendered. Currently, it is not possible to update the `initialUserConfig` config object, since any update to the `config` object is ignored. These updates in plugins are visible to other plugins, but not to the rendered document. There is a way to update the object with `"amend-user-config"`, but that requires publishing to the internal system. That system isn't accessible to a plugin. Therefore, add a new method that can be called with an `Object` that forwards it to the internal publication system. That way, a plugin can update configuration (like some of the core plugins do) and update the `initialUserConfig`.
001cf3d to
04fa3c6
Compare
|
I don't think these test failures are related to my change |
|
@TimvdLippe, should be ok after rebase. |
marcoscaceres
left a comment
There was a problem hiding this comment.
The use case is legitimate and the implementation is correct — pub("amend-user-config", ...) is exactly what core plugins use (e.g. caniuse.js), and routing it through makePluginUtils is the right pattern.
A few things worth addressing:
Type is too wide
/** @type {(configUpdates: Object) => void} */Object (uppercase) is the constructor type and accepts null in some contexts. The other entries in makePluginUtils use more specific types. Suggest:
/** @type {(configUpdates: Record<string, unknown>) => void} */Missing test
There's no test verifying that calling utils.amendConfiguration({ key: "value" }) from a preProcess function results in that key appearing in #initialUserConfig. The existing include-config-spec.js tests would be the right place to add one.
Missing docs
preProcess and postProcess docs mention the utils argument but don't document what's on it. Worth adding amendConfiguration to those pages (and the wiki) so authors know it exists.
The type nit and docs are minor — happy to see this land once there's a test.
|
Your comments appear to contradict each other. Based on the first comment, I would assume that a rebase is sufficient. However, your second comment (presumably generated by AI) implies that more work is needed. Then your third comment only picks one of the points of the AI comment, which I need to do after the PR merge. Can you please give me guidance which action I should take? |
|
Lol, sorry! the first comment was right. Deleted the second. |
|
Instead, I added to the existing integration test for preProcess plugins. |
|
I’ll take a look |
|
Do you mind doing that as a follow-up so that we can merge as-is? The implementation at least looks good |
There was a problem hiding this comment.
Pull request overview
This PR enables pre-/post-process plugins to update the document’s emitted initialUserConfig (via the existing internal amend-user-config pubsub topic) by exposing a new helper on the plugin utils object.
Changes:
- Add
utils.amendConfiguration()tomakePluginUtils()to publish config updates to"amend-user-config". - Extend the pre-process spec fixture to call
amendConfiguration()duringpreProcess. - Add a spec assertion that
initialUserConfigreflects the amended configuration.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tests/spec/core/pre-process-spec.js | Adds a test asserting initialUserConfig contains the amended value. |
| tests/spec/core/pre-process-spec.html | Adds a preProcess function that calls utils.amendConfiguration() with updates. |
| src/core/utils.js | Exposes amendConfiguration() on the plugin utils object via pubsub. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
Hey @TimvdLippe — could you rebase onto main? I'll merge once it's up to date. |
|
Sure, clicked the update button. |
|
Gah, sorry, one more time (I needed to land a hotfix before). I think there’s also a checkbox somewhere in GitHub that allows me also to do the updates. |
|
https://github.com/orgs/community/discussions/5634
That's the case with ours, since I created it from the Logius-Standaarden fork :/ |
|
All good... merged to avoid the rebase ping pong. |
We have a
preProcessplugin that needs to store some information for later use when a document is rendered. Currently, it is not possible to update theinitialUserConfigconfig object, since any update to theconfigobject is ignored. These updates in plugins are visible to other plugins, but not to the rendered document.There is a way to update the object with
"amend-user-config", but that requires publishing to the internal system. That system isn't accessible to a plugin.Therefore, add a new method that can be called with an
Objectthat forwards it to the internal publication system. That way, a plugin can update configuration (like some of the core plugins do) and update theinitialUserConfig.