Snapshot retain and asyncMap

[wuzzeb]

I recently updated to 0.3.1 and started getting the warning about calling snapshot.retain. For some background, I am using recoil in an electron app and when opening a file have code like follows

  const onOpenFile = useRecoilCallback(
    ({ snapshot, gotoSnapshot }) =>
      async function onOpenFile() {
        const releaseSnap = snapshot.retain();
        try {
          gotoSnapshot(await snapshot.asyncMap((mutSnapshot) => openFile({ newWindow: true, snapshot: mutSnapshot })));
        } catch (e) {
          // some code to handle errors here
        } finally {
          releaseSnap();
        }
      },
    []
  );

openFile is an async function which opens the file chooser dialog and once the user selects a file, loads and parses the file, and then sets various values into the mutSnapshot.

I started getting the warning and then added the calls to snapshot.retain that you see above, but the warning stayed. Adding some breakpoints into recoil, the error is actually being generated by the mutSnapshot created by asyncMap.

Should I be calling retain() on the mutSnapshot inside the asyncMap function? So code like

await snapshot.asyncMap(async (mutSnapshot) => {
  const releaseMut = mutSnapshot.retain();
  try {
    await openFile({newWindow: true, snapshot: mutSnapshot});
  } finally {
    releaseMut();
  }
}

It would be nice if asyncMap did this itself so that I don't have to. That is, if snapshot.retain() is called, the mutSnapshot is also retained.

[drarmstr]

(Note that for 0.3.1 this is only a warning and won't yet cause an issue. Thank you for reporting this usage pattern)

@davidmccabe - What do you think about inheriting the retain status of a Snapshot when mapping?

@wuzzeb - Do you need Snapshots here? Is this so openFile() can atomically change several atoms? Are you concerned that other state may mutate in the meantime and those changes be lost by going to the snapshot? We're looking into a mechanism here to atomically update several atoms that may be more appropriate.

[wuzzeb]

Yeah, there are about 25-30 atoms that are set during openFile from the contents of the file. I originally used a snapshot just to avoid having setters for each of them. Looking closer, I think it could be refactored to use a set-only selector.

const importFromFile = selector<DataFromFile | null>({
  key: "import-from-file",
  get: () => null,
  set: ({set}, dataFromFile) => {
    if (!dataFromFile || dataFromFile instanceof DefaultValue) return;
    // code here to set all the atoms
  }
});

All those sets should be batched and appear together. And then in the React component something like

const setFromFile = useSetRecoilState(importFromFile);

const onOpenFile = React.useCallback(async () => {
  const data = await openFile({newWindow: true});
  setFromFile(data);
}, [setFromFile]);

Thinking more, this is probably better than using a snapshot in this specific case.

---

But for the original question of asyncMap, I would think that anytime you use asyncMap that you want to keep the snapshot around, so asyncMap should retain itself.

[drarmstr]

Yeah, will discuss inheriting the retain status, it sounds reasonable.

Be on the lookout for the atomic updater proposed for the useRecoilCallback() interface that sounds appropriate for your case.

[davidmccabe]

Thanks for the feedback @wuzzeb

I think we should retain the mutable snapshot until the tick after asyncMap returns, regardless of whether the parent snapshot was retained. This is because it never makes sense to return a released snapshot from asyncMap; you can't do anything with it, so why would you have computed it?