-
Notifications
You must be signed in to change notification settings - Fork 62
Configuration API #504
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
base: main
Are you sure you want to change the base?
Configuration API #504
Conversation
samuelpecher
left a comment
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.
Nice. I like that configuration can still be done on the element directly. Just a couple comments on attachment tag name and a potential listener issue.
Overall I think we should build a way to provide the config from Rails as well.
Lexical Extensions! Should we consider refactoring parts of Lexxy into a set of extensions?
This feels like the right direction! I was going to try it with the next feature to make sure we can still keep the nice OO-patterns established in Lexxy.
|
|
||
| createDOM() { | ||
| const figure = createElement("action-text-attachment", { "content-type": this.contentType, "data-lexxy-decorator": true }) | ||
| const figure = createElement(lexxyConfig.get("global.attachmentTagName"), { "content-type": this.contentType, "data-lexxy-decorator": true }) |
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.
What do you think about passing the tag name to the Node constructor and keeping as internal node state rather than hooking into the config at every step?
Same would apply to ActionTextAttachmentNode
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.
I like it! I noticed you did the same for custom-upload-events, and considered including it here. But I also wanted to keep this minimal.
If you are expecting to merge custom-upload-events soonish I'd rather not introduce it here, otherwise I'm ok to include it.
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.
I'm splitting that branch into separate PRs back to main, so I'll handle that then 👌
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.
This would be a quite edge case but if we kept it in the internal node state, it would be copied/pasted carrying the tag, which assumes that the editor where you are pasting the node has the same override. I would keep it simple with a global access I think.
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.
@jorgemanrubia: the exchange format will be text/html unless the editors share the same namespace. The rendered HTML will include the rendered tag name and I think we should match that behavior.
Though I do propose that differently configured Lexxy editors shouldn't share a namespace and we should set it depending on the loaded config:
// ...
namespace: `Lexxy.${configName}`,
jorgemanrubia
left a comment
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.
Good spike @zachasme!
We need to add tests here. So far, Capybara has served us well. But here, to test the configuration interfaces and such, I think we could add jest to Lexxy. I would still rely on Capybara as our main testing mechanism, but it is nice to have more traditional unit testing available too. We could configure in the local CI script and in GH actions too. That would let us test the Configuration objects properly here.
I would still add some high level system tests to make sure the configurations work as expected. I would be fine with just testing the overrides in capybara, assuming the system is tested through jest. WDYT?
src/config/dom_purify.js
Outdated
| ALLOWED_TAGS: ALLOWED_HTML_TAGS, | ||
| ALLOWED_ATTR: ALLOWED_HTML_ATTRIBUTES, | ||
| SAFE_FOR_XML: false // So that it does not strip attributes that contains serialized HTML (like content) | ||
| lexxyConfig.listen("global.attachmentTagName", (attachmentTagName) => { |
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.
Why are we observing changes here instead of just grabbing the configuration directly? Is this a matter of changing the initialization order or something like that?
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.
This is necessary if we want modify the attachmentTagName using Lexxy.configure. Otherwise DOMPurify will remain configured with the default value.
Alternatively, DOMPurify.sanitize accepts a second configuration argument. We could avoid listening for changes if we build a fresh configuration for each call.
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.
I was thinking of mirroring Trix's implementation: call setConfig before each call. This allows a known good configuration and keeps the sanitize call clear.
Then we can 'pull' config and not need the listeners
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.
Sorry I still don't understand.
We configure dompurify when running:
import "./config/dom_purify"Wouldn't this be a matter of requiring the global configuration before that code runs, and then making dom purify use the global configuration?
|
|
||
| get #hasToolbar() { | ||
| return this.getAttribute("toolbar") !== "false" | ||
| return Boolean(this.config.get("toolbar")) |
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.
Is this keeping the default true value?
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.
Yes. The Boolean call technically isn't required, but it felt right since you can pass an element ID to toolbar.
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.
I still think it is redundant. Isn't JS semantics enough for evaluating true/false enough here? I would only add additional code here if it is really justified. It confuses me a bit whenever I see it.
src/config/configuration.js
Outdated
| return node | ||
| } | ||
|
|
||
| merge(config, into = this.#tree) { |
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.
I think the into param is a bit confusing. Why would you like to change that to other thing? This is an instance method, it is expected it will act on its private data. I see you use it recursively but it kind of makes the interface less clear. I would consider extracting a deepMerge helper method to helpers/hash_helper.js and reuse here. You could move arePlainObjects as a private (not exported) function there too.
|
|
||
| node = node[key] | ||
| if (node === true || node === false) { | ||
| return node |
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.
I think this should be more agnostic about the boolean value. It should loop through the path and return whatever it is at the end. Why that return?
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.
The intention was to eventually support different levels of granularity such as
{ toolbar: true }
// or
{
toolbar: {
bold: true
}
}Such that querying for "toolbar.bold" would return true in both cases.
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.
Hmm not sure about this, it does not look super intuitive. I think it is important that we keep the core config abstraction lean and generic. I would rather interpret this at toolbar-rendering time: assume that everything is on unless there is a FALSE in the settings.
src/config/configuration.js
Outdated
| return into | ||
| } | ||
|
|
||
| listen(path, listener) { |
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.
As mentioned, the listening mechanism seems a bit excessive to me. This is configuration stuff. It should be initialized and used, but why would you like to subscribe to dynamic changes?
src/editor/configuration.js
Outdated
| } | ||
|
|
||
| get(path) { | ||
| if (this.#preset) { |
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.
I would define a getter get config at the editor level, that returns the value of the attribute or "default" if the attribute is not set. Then, this can be deterministic. You just get the value of editorElement.config. You can rename the current config to configuration to remove the collision.
src/editor/configuration.js
Outdated
|
|
||
| get(path) { | ||
| if (this.#preset) { | ||
| return super.get(path) ?? lexxyConfig.get(`${this.#preset}.${path}`) ?? lexxyConfig.get(`default.${path}`) |
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.
Why checking the path before prepending the prefix path segment? I think this should be more deterministic. It should invoke the parent with the path prepending the editorElement.config segment.
I am not sure about the inheritance here. Configuration feels like a simple data structure you use. I think EditorConfiguration could have an attribute configuration that it uses to delegate the get and set, but inheriting from it feels slightly off to me.
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.
Why checking the path before prepending the prefix path segment? I think this should be more deterministic. It should invoke the parent with the path prepending the editorElement.config segment.
I wanted to support three possible levels of configuration: element attributes toolbar=false, editor presets config=simple, and defaults. Unless there is no preset selected in which case we only check element attributes and defaults.
I am not sure about the inheritance here. Configuration feels like a simple data structure you use. I think EditorConfiguration could have an attribute configuration that it uses to delegate the get and set, but inheriting from it feels slightly off to me.
Hm, yes, I'll try that!
src/editor/configuration.js
Outdated
|
|
||
| get #overrides() { | ||
| const overrides = {} | ||
| for (const attribute of EditorConfiguration.attributes) { |
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.
Instead of having to maintain the list of attributes in this place too, I would grab the list from the defaults Object.keys(configuration.get("defaullt")). This also adds some basic protection against typos in configuration keys when overriding them.
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.
Ooh nice! 👌
| } | ||
| } | ||
|
|
||
| return node |
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.
If the configuration key is missing, I wonder if this should throw an error. I think it should, to prevent entering typos when trying to override settings, which could be hard to debug.
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.
Hm, that's a good idea. Currently I'm returning null so I can detect and fallback to preset/default in EditorConfigutaion. But I could validate against the structure in default if we keep all possible configs in there.
| } | ||
|
|
||
| #parseAttribute(attribute) { | ||
| const value = this.#editorElement.getAttribute(attribute) |
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.
You could use JSON.parse instead of the ad-hoc booelan parsing. That way, it will work for boolean value types supported by JSON. This could be handy when providing Rails attributes during rendering.
|
Thank you for the feedback @samuelpecher and @jorgemanrubia! 👌
Agreed. I wanted to hear your thoughts on the general approach before writing new tests.
Yes, I think adding jest is justified at this point. Though I'd rather tackle that in a separate PR. Until we introduce more granular options for toolbar/etc, I believe I can cover most of the configuration system with some high-level system tests. |
|
To me, the motivation of jest would be to test the system we are bootstrapping here, more than the granular options themselves. I think those tests belong to this PR. |
Makes sense. Alright, I'll set up jest here. @samuelpecher I'll give |
| return this.isEmpty || this.toString().match(/^\s*$/g) !== null | ||
| } | ||
|
|
||
| get preset() { |
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.
Since you configure the element with config= it makes sense to me that this is get config instead of using a new name. We do that with other attributes lke name. Feels more consistent. Any reason to use a different name here?
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.
No reason, they should definitely be consistent. I've been back and forth on config/preset. I prefer preset, it feels more descriptive than the generic config.
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.
But config is what we expose as the public API to be used by the editor users. What is confusing to me is using different terms here:
<lexxy-editor config="whatever"I would expect that if you do editorElement.config returns whatever. Now, we want to make that, if config is not set, then its default value is default, so that you can query editorElement.config consistently in any scenario. See what I mean?
We could also change the public api method to preset, but I think it is important that things remain consistent here. I like config slightly better because we are talking about configurations when defining these.
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.
Maybe we could go with presets indeed at the configuration level. So the lexxy configuration could expose two different structures:
{
global: new Configuration(...)
presets: new Configuration({
default: {
}
...
})
}So that we use preset at the public API (setter) and as a getter in the element.
And we make global a explicit configuration object to query like config.global.get("tagName") or whatever. And same for the presets: config.presets.get("default.whatever") or just config.presets.default to grab the properties we want for a preset.
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.
So, for creating a editor-specific config we could do:
this.#config = new Configuration(lexxyConfig.presets.default)
this.#config.merge(this.#overrides)Thinking we could even extend the configuration constructor to admit a list of configurations (...configurations) that it will merge to avoid the explicit merge:
this.#config = new Configuration(lexxyConfig.presets.default, this.#overrides)I think that is pretty clean. The code expresses what we are trying to do in a single line.
src/editor/configuration.js
Outdated
|
|
||
| constructor(editorElement) { | ||
| this.#editorElement = editorElement | ||
| this.#config = new Configuration(this.#overrides) |
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.
I think like this should be something like
this.#config = new Configuration(lexxyConfig.get("default"))
this.#config.merge(this.#overrides)So that this.#overrides only returns the set of properties overridden by the editor. And then, you can use this.#config deterministically. I think the get having to check whether it is an override or not is a smell . Our config data structure allow overrides via merge, we should leverage that here, I believe.
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.
In my mind the editor configuration would reflect any changes made to defaults/presets after editor instantiation, hence the reference directly in get.
But I realize now this is probably worse, not being able to assume a consistent config during an editor lifecycle.
Removing the runtime look-ups should make things much simpler!
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, I think it is totally fine to assume that the properties are grabbed when the editor is instantiated, and subsequent changes won't be grabbed. That seems like a reasonable expectation to establish.
src/elements/editor.js
Outdated
| if (!this.cachedValue) { | ||
| this.editor?.getEditorState().read(() => { | ||
| this.cachedValue = sanitize($generateHtmlFromNodes(this.editor, null)) | ||
| this.cachedValue = sanitize($generateHtmlFromNodes(this.editor, null), { additionalAllowedTags: [ lexxyConfig.get("global.attachmentTagName") ] }) |
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.
If this is set globally for dompurify, we don't need to do this right?
| static importDOM() { | ||
| return { | ||
| "action-text-attachment": (attachment) => { | ||
| [lexxyConfig.get("global.attachmentTagName")]: (attachment) => { |
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.
Checking these usages, I think making global a dedicated conttainer would read slightly better. And I think it is right to make global its own thing.
lexxyConfig.global.get("attachmentTagName")| import Configuration from "../../config/configuration" | ||
|
|
||
| test("gets path", () => { | ||
| const c = new Configuration({ |
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 normally avoid single-letter identifiers. Better to go with config or configuration.
| @@ -0,0 +1,34 @@ | |||
| import { expect, test, vi } from "vitest" | |||
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.
Great to have JS tests available now 👍. Please configure an entry in Github actions and our local ci setup to run them as part of our pipeline.
| test("uses defaults", () => { | ||
| lexxyConfig.merge({ | ||
| default: { singleLine: true } | ||
| }) |
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.
Here we are modifying a global data structure as part of a single test run, which doesn't look great. Also, these tests are stubbing some internals. Ideally we would test the real thing here.
I think we could modify EditorConfiguration to receive as a second argument the presetConfiguration to use (with a default) so that, each test can use a fresh data structure for the defaults. I think these should be more like a black box: I have these defaults and I pass this element. I think we could also use jsdom to create a proper dom element from some HTML string for testing this? That way we could get rid of stubbing hasAttribute, which is an internal implementation detail.
…, merge all config constructor args into tree
f2e0451 to
322cbb7
Compare
|
Thanks for the thorough feedback @jorgemanrubia! The PR might have been a bit too WIP, but the responses here are really clearing things up for me. It's slowly coming together nicely. I think this is an important discussion: { toolbar: true }
// or
{
toolbar: {
bold: true
}
}
I agree we should assume everything is on by default, fully defined by the default preset. But I also think we should allow users to configure({ default: { toolbar: false }})In cases where a bool is overriding a hash, I think it would be valuable to recursively write the bool into nested hashes. It will complicate the merge logic, but it will make Alternatively, we can keep the config class completely generic and rely on the rest of the codebase to handle these differences.
I think they both have merit. In isolation
I've also changed our workflow .yml to run on all pushes. Was there a specific reason we previously had: on:
pull_request:
push:
branches: [ main ] |
jorgemanrubia
left a comment
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.
Looking tighter @zachasme 👏
| @@ -0,0 +1,5 @@ | |||
| import { JSDOM } from "jsdom" | |||
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.
I think I would prefer to keep test/ as a first level folder, parallel to src/. I think it is cleaner, so that it doesn't look like another system inside the source. I see that in trix it was inside, but in trix we also had a top level src/trix folder that we don't have here.
Also, I would move the dom_helper.js to a folder test/helpers.
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.
I've moved them into test/javascript/. 👍
My thinking was we wanted them separate from the rails tests, but I agree this is cleaner.
|
|
||
| test("uses defaults", () => { | ||
| const element = createElement("<lexxy-editor></lexxy-editor>") | ||
| const config = new EditorConfiguration(element, "default") |
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.
With the new implementation, we don't have the problem I mentioned of having tests altering the same global structure over and over. I would remove the second param (preset) in the editor configuration constructor, since we don't need to override the config for testing purposes anymore. In the tests, I would insert that preset at the element level createElement("<lexxy-editor preset="simple"></lexxy-editor>") or just omit it there if you want to pass the default.
const element = createElement("<lexxy-editor></lexxy-editor>")
const config = new EditorConfiguration(element)
// or, for a specific one:
const element = createElement("<lexxy-editor preset="simple"></lexxy-editor>")
const config = new EditorConfiguration(element)That way we would be testing the real thing.
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.
Indeed. I kept the param because I had trouble getting jsdom to play nice with custom elements. I found a solution, though, and the param is gone.
|
|
||
| get #overrides() { | ||
| const overrides = {} | ||
| for (const option of Object.keys(DEFAULT_PRESET)) { |
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.
I would rather keep the DEFAULT_PRESET as an internal constant and keep the configuration interface as the only way of interacting with the configurations. So I would use lexxyConfig.presets.get("default") instead of depending on the constant.
| presets: new Configuration({ default: DEFAULT_PRESET }) | ||
| } | ||
|
|
||
| export function configure(changes) { |
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.
Since this is a function to import, maybe configureLexxy instead of just configure would be more convenient.
The thing is that I would prefer an OO interface, so that you would do:
Lexxy.configure(...)
Lexxy.config.global
Lexxy.config.presets.get(...)I think that feels more consistent and offers a proper namespace for other stuff we may add down the road. This way also you don't have to figure out how to rename lexxy functions and identifiers: lexxyConfig, lexxyConfigure... they are properly namespaced.
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.
If we're talking public interface, we could keep the existing named exports and also provide a convenience default export, so the following would work the same:
import * as Lexxy from "lexxy"
// same as
import Lexxy from "lexxy"
// usage
Lexxy.configure(...)
Lexxy.highlightAll()We could also assign the same object to a global window.Lexxy. What do you think?
In terms of internal usage, how about exporting the following from config/lexxy.js?
export default {
global,
presets,
configure(changes) {
return presets.merge(changes)
}
}
// usage
import Lexxy from "./config/lexxy"
Lexxy.global.get(..)And then re-export Lexxy.configure for the public interface.
|
|
||
| connectedCallback() { | ||
| this.id ??= generateDomId("lexxy-editor") | ||
| this.config = new Configuration(this, this.preset) |
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.
Mentioned in other place, but passing an object and an attribute of that object as params feel off. It is an editor configuration: receiving the editor element should be enough. Notice the original mechanism I had suggested for tests was intended for tests only, as a mechanism to override the default param.
| @@ -1,3 +1,11 @@ | |||
| export function camelize(value) { | |||
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.
this function is not used right? I think we could nix
| this.#editorElement = editorElement | ||
| this.#config = new Configuration( | ||
| lexxyConfig.presets.get("default"), |
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.
Mentioned above: I would remove the preset second argument here. It is an editor config that receives an editor element, totally fine to query the editor element for its preset now:
this.#config = new Configuration(
lexxyConfig.presets.get("default"),
lexxyConfig.presets.get(editorElement.preset),
this.#overrides
)
I would wait to see if we need the extra complication. If someone sets that the toolbar is |
|
On @zachasme and we also need to document the new configuration API in the README 🙏 |
Good point, let's keep it simple.
Almost there! |
Based off suggestions from @jorgemanrubia, this PR allows configuring lexxy instances in three layers:
I took into consideration the work on custom-upload-events, which will require:
importDOMAPI.Note that I'm only exposing the
configurefunction as public API, but we could include a getter if end-users need to read current config values.As for next steps:
{toolbar: true}and{toolbar:{bold:false}}.