Skip to content
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

Use a single settings file and refactor settings #2640

Merged
merged 7 commits into from Mar 4, 2024

Conversation

RunDevelopment
Copy link
Member

This PR completely changes how we do settings. Instead of the weird local storage thing we did, all settings are now stored in a single settings.json. The main process is responsible for reading and writing settings.json. The renderer can read and write settings via ICP channels.

The main advantages of this architecture are:

  1. It's clear who owns settings (the main process). We previously had this weird hand-off where the render process suddenly gained ownership of the settings.
  2. A single settings.json is easier for users to edit and for us to change (settings migration).
  3. Settings are now strongly typed, and there is only a single parser for settings. This makes settings easier to use since we don't have to remember the keys anymore.
    Turned out, backend package settings were actually parsed incorrectly in CLI mode. Not anymore.

Since the new settings use a single settings.json, there is a migration from the old format (see tests for an example). This migration is the old reason why node-locastorage is still a dependency. We can remove it in the future, but it's still necessary right now for the backwards compatibility.

I also added something called "storage." Storage is accessed using the useStored(key: string, default: T): T hook. This is basically a drop-in replacement for the old useLocalStorage hook. The main difference is that useStored will store its values in settings.json.
The difference between storage and settings is that settings are used across UI components while stored values are basically useState of a single component that is remembered across restarts (e.g. when the node selector is open or closed). Storage is less structured.

Tbh, I'm not too sure about storage. This could just be localStorage, but I'm not sure how that would work with migrating settings. Maybe we'll change that in the future. Idk.

One thing that I haven't tested enough yet is performance. Since settings are completely reactive now, there can be more re-renders than before. This can cause perf problems. The node selector optimization PRs I recently did were because of this. Perf seems to be the same as without this change, but this still needs more testing.


It goes without saying, but this is a deep change. We should probably release this at the same time as #2597.

@joeyballentine
Copy link
Member

Looks like this works, and I like the new elegance of the settings context. Is this ready for review now?

@RunDevelopment
Copy link
Member Author

Yes. It seems to work fine in my testing.

@RunDevelopment RunDevelopment marked this pull request as ready for review March 4, 2024 22:46
@joeyballentine joeyballentine merged commit f984e78 into chaiNNer-org:main Mar 4, 2024
7 checks passed
@RunDevelopment RunDevelopment deleted the setting2 branch March 5, 2024 02:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants