plugin configs + brimcap yaml config/preference#1637
Conversation
jameskerr
left a comment
jameskerr
left a comment
There was a problem hiding this comment.
Looking good!
It looks like the "value" of a config is stored in the plugin storage slice and in the config slice (as the default value). I also see a new command created for each config property which is like an "onChange" listener.
I propose we store the config settings in a state slice (like you have), but save the config values in another separate slice of state. (config values). Config values will be persisted. The form could handle updating the config values automatically whenever it gets submitted. Then we wouldn't need to manually save the values in storage or update the default value. We also wouldn't need the custom command.
If we did that, the brimcap plugin would become...
// brimcap-plugin.ts
setupConfig() {
this.api.configs.add({
name: this.pluginNamespace,
title: "Brimcap Settings",
properties: {
[configPropertyName]: {
name: configPropertyName,
type: "file",
label: "Brimcap YAML Config File",
defaultValue: "" // only used in the form if the config value is unset
}
}
})
}The configs api could become...
class ConfigsApi {
add(config: Config) {
this.dispatch(Configs.create(config))
}
update(key, value) {
this.dispatch(ConfigValues.set(key, value))
}
get(key) {
ConfigValues.get(key)(this.getState())
}
}And the useConfigsForm hook would change the submit function to...
// usePreferencesForm.ts line 28
const submit = (value) => dispatch(ConfigValues.set(key, value))I like the idea of having the values all stored together and managed by the preferences form. As a plugin, I just add the configuration object and I know that whenever I ask for the value, it will be up to date.
I like the storage state slice and api, so let's leave that. It can be used for any data that the plugin wants to keep around and it will be responsible for updates.
What do you think?
| } | ||
| }) | ||
|
|
||
| return <>{formInputs}</> |
There was a problem hiding this comment.
| return <>{formInputs}</> | |
| return formInputs |
| const api = new BrimApi() | ||
| const store = await initStore(api) | ||
| api.setStoreArgs(store.dispatch, store.getState) | ||
| api.init(store.dispatch, store.getState) |
src/js/api/ui-apis.ts
Outdated
| this.dispatch(Configs.create(config)) | ||
| } | ||
|
|
||
| public updatePropertyDefault( |
There was a problem hiding this comment.
| public updatePropertyDefault( | |
| updatePropertyDefault( |
src/js/api/ui-apis.ts
Outdated
| export class ConfigsApi { | ||
| constructor(private dispatch: AppDispatch, private getState: () => State) {} | ||
|
|
||
| public add(config: Config) { |
There was a problem hiding this comment.
| public add(config: Config) { | |
| add(config: Config) { |
Signed-off-by: Mason Fish <[email protected]>
Signed-off-by: Mason Fish <[email protected]>
Signed-off-by: Mason Fish <[email protected]>
Signed-off-by: Mason Fish <[email protected]>
Signed-off-by: Mason Fish <[email protected]>
mason-fish
force-pushed
the
1545-brimcap_preferences
branch
from
cc65fe0 to
e3b6487
Compare
Signed-off-by: Mason Fish <[email protected]>
jameskerr
left a comment
jameskerr
left a comment
There was a problem hiding this comment.
This is coming together! I left a few suggestions for ya. The most important to me is saving the config values as a flat object, instead of nested by name. What do you think of that?
| } | ||
|
|
||
| const slice = createSlice({ | ||
| name: "$configPropValues", |
There was a problem hiding this comment.
What do you think about calling this just "ConfigValues" ?
| ConfigPropValues.set({ | ||
| configName: configName1, | ||
| propName: configProperty1, | ||
| value: data1 | ||
| }) |
There was a problem hiding this comment.
What do you think about making the config values a flat object, and we trust that the keys will be prefixed properly? I think it would make for a more clear, less verbose api if it looked like: ConfigValues.set(key, value)
| export type ConfigItemType = "file" | "string" // | "number" | "boolean" | ||
|
|
||
| export type ConfigItem = { | ||
| name: string |
There was a problem hiding this comment.
Maybe this name doesn't need to be stored? Since the key and the name are the same and should be in sync?
2 participants
fixes #1545, #1593
This PR extends the brimApi to expose a generic persisting storage for plugins, and adds a new UI api which corresponds to what we currently see as preference options.
Following a bit of the naming choices made in VSCode, I have named this new api and its new backing storage/reducer
configs, and have kept the remainingprefsthat are not related to brimcap in their current place. We should eventually move those into a core plugin and store them under the newconfigsas well, so that we may fully transition away from theprefsstate.As it stands, the new redux slices we've added to facilitate plugins contributing ui elements (
toolbarsfirst, and nowconfigs, and soon we will want something forcontextMenustoo) are NOT persisted. Every time the app starts up, plugins will populate and maintain these slices via the brimApi, the app will "react" to those state changes appropriately, and then when the app quits it will not save that ui state. What I like about this pattern is that by not persisting that plugin-generated state, and by regulating it's manipulation through the brimApi, we have more flexibility to make changes to the underlying app state shape without breaking things or requiring migrations.This does create a need for plugins to persist their own data though, hence including this new generic
storageobject on the brimApi. For now it functions as a simple key value store, where the key is a plugin namespace and the value is arbitrary. I'd like to eventually protect this better so that plugins cannot access each other's storage which is currently possible, and when the need arises we could also enhance the storage to optionally use the keychain for a more secure persisting storage. This would then align the api pretty well for transitioning the auth related work into it's own plugin too.