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

Prevent reused selections from useShape when using the selector argument #1446

Closed
KyleAMathews opened this issue Jul 26, 2024 · 0 comments · Fixed by #1615
Closed

Prevent reused selections from useShape when using the selector argument #1446

KyleAMathews opened this issue Jul 26, 2024 · 0 comments · Fixed by #1615
Labels

Comments

@KyleAMathews
Copy link
Contributor

I spent a while debugging this. Basically the problem is that when I selected some data from a shape in one route of my app e.g. /page/1, the selected data is what my selector saw when I went to /page/2. Since I was filtering the array of rows down to a single row, this broke my selector.

The problem is two-fold.

  1. we're using useRef https://github.com/electric-sql/electric-next/blob/340596deba607494251db565be82141a5782ff22/packages/react-hooks/src/react-hooks.tsx#L150 — which caches data across all instances of a hook as I understand it.
  2. I'm building my app in Remix which re-uses the same component when switching routes on the same route component — so the same hook instance of useShape is used.

To mitigate, you can for now just clone the object passed to the selector and mutate that. Avoid directly mutating the shape data. In the future we could two things to make this simpler to manage.

  1. Don't use useRef.
    Instead use per shape instance cache. I prototyped something like this:
function useCachedValue(initialValue) {
  let cachedValue = initialValue; 

  const getCachedValue = () => {
    console.log(`getCachedValue`, cachedValue)
    return cachedValue;
  }

  const setCachedValue = (newValue) => {
    cachedValue = newValue;
  };

  return [getCachedValue, setCachedValue];
}
var identity = (arg) => arg;
function useShape(_a) {
  var _b = _a, {
    selector = identity
  } = _b, options = __objRest(_b, [
    "selector"
  ]);
  const { getShape: getShape2, getShapeStream: getShapeStream2 } = useShapeContext();
  const shapeStream = getShapeStream2(options);
  const shape = getShape2(shapeStream);
  // const latestShapeData = useRef(parseShapeData(shape));
  const [latestShapeData, setLatestShapeData] = useCachedValue(parseShapeData(shape));
  // const latestShapeData = useMemo(() => parseShapeData(shape), [shape.valueSync.values()]);
  console.log({latestShapeData: latestShapeData()})
  const getSnapshot = React.useCallback(() => latestShapeData(), []);
  const shapeData = useSyncExternalStoreWithSelector(
    useCallback(
      (onStoreChange) => shapeSubscribe(shape, () => {
        setLatestShapeData(parseShapeData(shape));
        onStoreChange();
      }),
      [shape]
    ),
    getSnapshot,
    getSnapshot,
    selector
  );
  return shapeData;
}

Second we could allow a dependencies array which resets the shapeData whenever it changes so e.g. in my case you could call:

useShape(shapeDef, [params.id])
@KyleAMathews KyleAMathews transferred this issue from electric-sql/archived-electric-next Aug 6, 2024
@balegas balegas added the bug label Aug 26, 2024
KyleAMathews pushed a commit that referenced this issue Sep 3, 2024
…ges (#1615)

Addresses #1446

There is some weirdness with the `useSyncExternalStoreWithSelector` hook
when passed a ref w.r.t. to selector changing - need to look into this
deeper, I suspect memoization is using referential equality checks which
is messing things up.
KyleAMathews pushed a commit that referenced this issue Sep 3, 2024
This PR was opened by the [Changesets
release](https://github.com/changesets/action) GitHub action. When
you're ready to do a release, you can merge this and the packages will
be published to npm automatically. If you're not ready to do a release
yet, that's fine, whenever you add more changesets to main, this PR will
be updated.


# Releases
## @electric-sql/[email protected]

### Patch Changes

- 3c8f662: Fix `useShape` not returning correct data upon changing
`selector` prop - see
<#1446>.
-   b0f39c7: add test that useShape re-renders on where state change

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants