-
Notifications
You must be signed in to change notification settings - Fork 70
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
Move Workbench settings to a new persistent data store #1359
Conversation
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.
@davemfish this all makes sense to me! Just had a few minor comments. Also it looks like we're removing the option for the user to reset settings to defaults. I think that's fine, I just didn't see it mentioned, so wanted to make sure it's intended to go in this PR.
import i18n from './i18n/i18n'; | ||
import pkg from '../../package.json'; |
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.
Unused?
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 used, just not in the diff
workbench/src/main/settingsStore.js
Outdated
type: 'object', | ||
properties: { | ||
nWorkers: { | ||
type: 'string', |
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.
Could this type be 'number'
? https://ajv.js.org/json-schema.html#type
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 you're right, it is better as a number. Any value posted through an html form will be a string though, so this required converting the value to a number in the change handler before saving it.
enum: ['CRITICAL', 'ERROR', 'WARNING', 'INFO', 'DEBUG'], | ||
}, | ||
language: { | ||
enum: ['en', 'es', 'zh'], |
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.
Would it be worth pulling these values in from invest using the getSupportedLanguages
endpoint?
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.
That's an interesting question. Normally I would say, yes, but after some thought I feel like it's safest to keep these hardcoded. If any unexpected values ever made it into this schema, then unexpected things might happen, and the whole app might crash. I guess we could be prepared to catch that, but I just feel more at ease by making that fail case impossible by using a statically defined schema rather than a dynamic one.
I kinda go back and forth; it's a hard one.
@emlys Oh yeah, I forgot to mention removing that reset settings button. It seemed not very useful since we have so few settings & options to choose from. And it would have gotten a little more complicated, probably needing to exclude the language from the reset. So it's gone for the sake of simplicity. I think I addressed your other comments. It's kind of a hard call on whether to get those language options from the server or keep them hardcoded in the schema. |
This PR builds off of #1168 by expanding our use of
electron-store
to manage Workbench settings. All settings are now managed this way instead of usinglocalForage
.The main objective was to make sure these data are always compatible with the version of the Workbench reading them, since the same store is used across multiple installations of the Workbench. So that's achieved by validating the store against a schema and setting default values for any missing properties or invalid values.
I made a few simplifications along the way:
n_workers
in the args dict. It felt needlessly complex to retrieve that value from the store and include it in these args dicts. Mainly because it now requiresipcRenderer.invoke
calls that would require mocking in tests. My feeling is that no one expects to see this parameter in the args dict anyway, since it does not appear in the model's form. I'm open to reversing this though.This partially addresses #1275 and #1286. I'll do a separate PR to complete those, which will involve doing some similar validation for the invest jobs data that is still being stored in
localForage
.Checklist