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

Recoil -> Jotai Conversion #467

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Conversation

StephenGrider
Copy link
Contributor

@StephenGrider StephenGrider commented Jan 1, 2025

Recoil has been deprecated + it doesn't work with React v19. This PR replaces Recoil with Jotai.

Outstanding issues that need to be fixed:

  1. Apparently Jotai doesn't support update functions for setters like React.setState and Recoil do. Instead of using an updater, I retrieved the atom's existing data ahead of time and then used it during the update. The risk here is updating with stale data. See comment on GraphList.tsx for example.
  2. The way recoil persist stored data in localStorage appears to be slightly different from jotai's. In Recoil you'd create a persist atom who's key dictated the localStorage key. Then you add that atom to other atoms you want persisted. Net effect is a structure like this:
// JSON representation of localstorage
{
   // "project" is the name of the key of the persist atom
   "project": {
      // persist atom was added to 'projectState' atom, adding this nested key
      "projectState": { ... },

      // persist atom was added to 'loadedProjectState' atom, adding this nested key
      "loadedProjectState": { ... }
   }
}

Jotai's persist has per-atom keys. Net effect of this: app will crash on load if you have any persisted data due to the unexpected structure. App will load if you clear local storage.

To solve this, you can either add in a localstorage migrator that runs on startup, detects if localstorage has unexpected structure of data and either migrates/deletes it. Alternatively, can get jotai to store data in the same nested fashion. This is the easier fix and its what I'll likely do.

@@ -429,33 +430,34 @@ export const GraphList: FC<{ onRunGraph?: (graphId: GraphId) => void }> = memo((
toast.error('A graph or folder with that name already exists.');
return;
}
setSavedGraphs(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recoil supports updater functions automatically, Jotai apparently doesn't. Have to get the savedGraphs ahead of time then use them to do the update. Risk here is using stale data.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I am worried about that, but as long as we test it to make sure there's nothing lost anywhere we're probably okay? A lot of it has to do with loading and saving graphs, and managing the "opened project" state

Copy link
Contributor Author

@StephenGrider StephenGrider Jan 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was incorrect regarding the jotai updater fn's. It is only an issue when you provide a custom setter function in the atom definition like this:

export const connectionsState = atom(
  (get) => get(graphState).connections,
  (get, set, newValue: NodeConnection[]) => {
    const currentGraph = get(graphState);
    set(graphState, { ...currentGraph, connections: newValue });
  },
);

When you use that atom in a component and call a setter on it, the new value passed in is required to be of type NodeConnection[]. This is why I thought jotai had issues with updaters.

You can allow flexibility by allowing for the newValue to be either the new value or an updater fn, like so:

export const connectionsState = atom(
  (get) => get(graphState).connections,
  (get, set, newValue: NodeConnection[] | ((prev: NodeConnection[]) => NodeConnection[])) => {
    const currentGraph = get(graphState);
    const currentConnections = currentGraph.connections;

    const nextConnections = typeof newValue === 'function' ? newValue(currentConnections) : newValue;

    set(graphState, { ...currentGraph, connections: nextConnections });
  }
);

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh cool!

@abrenneke
Copy link
Collaborator

I love this! Thanks! We'll need to address the local storage conflict problem first, but other than that, some testing and we should be good to go

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.

2 participants