Skip to content

Refactor StateStore to remove dependency on the atom global #1256

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

savetheclocktower
Copy link
Contributor

I had occasion to visit src/state-store.js and thought I'd refactor it a bit.

I know we have this annoying problem where we want to rely on the atom global, but we end up acting before it's defined. This is vexing to StateStore, since it wants to read from config to decide which kind of adapter to use (IndexedDB or SQL). In this case, we can pass the config instance into the constructor, dependency-injection–style. By the time we actually need to read the config, it's been initialized and is ready to read values.

The SQL adapter also needs to know where to store its DB, so it reads atom.getConfigDirPath(). This value can't be introspected from atom.config, and it isn't even ready by the time we create StateStore; it gets initialized later in the environment setup process. The fix is similar to the fix for a lot of similar objects created during bootstrapping: a second “initialization” phase where the later properties can get set. This is still early enough to be set before the first time we need to actually use StateStore, so it all works out.

This allows us to simplify the implementation of src/state-store.js and make some more methods synchronous. (They still go async later, but the code is easier to read this way.) We also no longer need to do the awkward thing where we check for the atom global every 50ms.

All the relevant tests seem to pass locally, but let's see what CI says.

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.

1 participant