-
Notifications
You must be signed in to change notification settings - Fork 8.7k
DEV: Introduce a value transformer front-end plugin API #27090
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
Conversation
07c56ec
to
a701373
Compare
app/assets/javascripts/discourse/app/lib/plugin-api/value-transformer.js
Outdated
Show resolved
Hide resolved
app/assets/javascripts/discourse/app/lib/plugin-api/value-transformer.js
Outdated
Show resolved
Hide resolved
header/user-dropdown/notifications
- Renamed to transformer - Removed DAG functionality - Added predefined list of transformers - Added API to register a new transformer name - Added test helper functions to clear registered transformer and transformer names that were added
Better comments and error messages
504512d
to
02cbf8b
Compare
* | ||
* @returns {*} the transformed value | ||
*/ | ||
export function applyTransformer(transformerName, defaultValue, context) { |
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.
One concern I have is that people are gonna pass something weird into 'context'.
e.g. they might do
applyTransformer("blah", value, this);
and pass an entire component instance. That would make any integrations quite fragile.
Could something like this help?
if(context && !(typeof context === 'object' && context.constructor === Object)){
throw "Context must be a simple JS object"
}
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.
We can add this, but TBH, it will just make it slightly harder.
If people want to pass the component in the context, they will.
It won't prevent them from doing something like:
applyTransformer("blah", value, { component: this });
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.
On second thought, there is nothing we can do about third-party abusing it, but since most of these extension points will live in core and/or official plugins.
Adding this error might just be enough to prevent us, from doing it, like a not so gentle nudge.
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.
Yeah agreed - it won't be perfect, but it should at least discourage it 🤞
This pull request has been mentioned on Discourse Meta. There might be relevant details there: |
This PR introduces the
valueTransformer
API to safely override values defined in Discourse.Two new plugin APis are introduced:
addValueTransformerName
which allows plugins and theme-components to add a new valid transformer name if they want to provide overridable values;registerValueTransformer
to register a transformer to override values.It also introduces the function
applyValueTransformer
which can be imported fromdiscourse/lib/transformer
. This function marks the desired value as overridable and applies the transformer logic.How does it work?
Marking a value as overridable:
To mark a value as overridable, in Discourse core, first the transformer name must be added to
app/assets/javascripts/discourse/app/lib/transformer/registry.js
. For plugins and theme-components, use the plugin APIaddValueTransformerName
instead.Then, in your component or class, use the function
applyValueTransformer
to mark the value as overridable and handle the logic:Overriding a value in plugins or themes
To override a value in plugins, themes, or TCs use the plugin API
registerValueTransformer
: