Issue when removing item

[Vincz]

Describe the bug
Hey Jared :) I found an issue when we remove element from the items list. If there is a lot of items, it seems to work. But if the list is small, we get an error.

To Reproduce
I made a sandbox to reproduce : https://codesandbox.io/s/masonic-example-lpe8s
Just click on the button "Remove a cat"
If you change the number of cat to 200 for example, it works.
With 20 cats, it failed with the following error:

TypeError
Invalid value used as weak map key
s2
https://lpe8s.csb.app/node_modules/trie-memoize/dist/main/index.js:65:14
eval
https://lpe8s.csb.app/node_modules/trie-memoize/dist/main/index.js:92:9
eval
https://lpe8s.csb.app/node_modules/masonic/dist/main/index.js:131:11
eval
https://lpe8s.csb.app/node_modules/masonic/dist/main/index.js:796:9
Object.search
https://lpe8s.csb.app/node_modules/masonic/dist/main/IntervalTree.js:374:35
range
https://lpe8s.csb.app/node_modules/masonic/dist/main/index.js:795:20

[jaredLunde]

This is the expected behavior, though I need to it document it better. This is the same problem solved by:

• https://codesandbox.io/s/masonic-w-react-router-example-2b5f9?file=/src/index.js
• https://codesandbox.io/s/masonic-w-react-router-and-advanced-config-example-8em42?file=/src/index.js

When you change your items list you are responsible for resetting the positioner. The reason for this is Masonic can't make assumptions about changes to the items array, since people could be using a list this mutable or immutable, that gets longer over time, etc.

[jaredLunde]

I should maybe throw a specific error in dev, too, if the items list gets shorter but the positioner doesn't change. Will have to explore that.

[renanbgarcia]

Should it work with the useMasonry() hook too?
I've been trying but it's still not working when the items array size change.

https://codesandbox.io/s/usemasonry-example-gvf4b

[jaredLunde]

@renanbgarcia Numerous problems in your example, but at the end of the day it's just not a good idea to do this. It's rather out of the scope of the library. Firstly, you're unlikely to get the kind of behavior you're looking for. No matter what, you're going to have to trigger a reflow on the entire grid, which means when a user deletes an item they'll end up back at the beginning of the grid each time it happens. You're much better off doing something else with the card when it's been deleted like inserting a placeholder letting a user know it was deleted. The primary objective of this library is to create masonry grids in an algorithmically-sound way. A secondary objective is keeping the hit to the bundle size low. Neither of those two things really mesh with this capability. There might be a day when I revisit this, but it's not on the current roadmap.

[khmm12]

@jaredLunde

In that case usePositioner should recreate positioner instance before masonic render when deps has been changed ;-).

masonic/src/use-positioner.ts

Line 42 in 6fca869

if (didMount.current) setPositioner(initPositioner())

What happens now:

1. The items list has been changed;
2. usePositioner schedules effect after component render;
3. useMasonic renders items using stale positioner instance;
4. React executes the scheduled effect, a new positioner is created;
5. useMasonic renders items using the new positioner instance.

I think it's clear what will happen on 3 step if the items list becomes shorter.

It should be something like that:

type UsePositionerState = {
  deps: React.DependencyList
  positioner: Positioner
}

const [state, setState] = useState<UsePositionerState>(() => {
  const positioner = initPositioner()
  return {
    deps,
    positioner
  }
})

let { deps: previousDeps, positioner } = state

const hasDepsChanged = !isEqual(previousDeps, deps)
if (hasDepsChanged) {
  const newPositioner = initPositioner()
  positioner = newPositioner
  previousDeps = deps

  setState({ deps, positioner })
}

It will guarantee new positioner instance during the same render.

[jaredLunde]

@khmm12 Thanks again for the detailed look into the implementation. Would you want to submit a PR for this? Something like the above looks good for now. In the next major it'd be nice to drop deps and accept an isEqual() function as the second argument instead.

[y0zhyk]

@jaredLunde, @khmm12 I don't think the issue it completely fixed. I continue to see the issue even after upgrade to v3.3.10.

[jaredLunde]

@y0zhyk post a reproduction on codesandbox please

[JhonJohnatan]

I agree. it didn't fixed.

[khmm12]

@y0zhyk @JhonJohnatan Could you share an example? It's hard to say anything helpful without code.

I have some assumptions, but it's better to see the example at first.

[cryptoAlgorithm]

@khmm12 @jaredLunde I've noticed the same issue even after updating to the latest version. I've forked the codesandbox example in the first post and updated Masonic to the latest version, but I receive the error

No data was found at index: 19

This usually happens when you've mutated or changed the "items" array in a way that makes it shorter than the previous "items" array. Masonic knows nothing about your underlying data and when it caches cell positions, it assumes you aren't mutating the underlying "items".

See https://codesandbox.io/s/masonic-w-react-router-example-2b5f9?file=/src/index.js for an example that gets around this limitations. For advanced implementations, see https://codesandbox.io/s/masonic-w-react-router-and-advanced-config-example-8em42?file=/src/index.js

If this was the result of your removing an item from your "items", see this issue: https://github.com/jaredLunde/masonic/issues/12

Screen.Recording.2021-05-17.at.10.44.39.PM.mov

Here's the forked codesandbox: https://codesandbox.io/s/masonic-example-forked-hcz62

[sebyx07]

@cryptoAlgorithm you can try the following thing:
when you delete a item item, you don't really remove it, just set a property like .__to_be_removed__ = true
then in you render FakeCard

if(__to_be_removed__) return null

not pretty, but works

[khmm12]

@cryptoAlgorithm

The same example but using masonic hooks directly.
https://codesandbox.io/s/masonic-example-forked-18nu4?file=/src/index.js

The key difference is usePositioner's dependency.

  const positioner = usePositioner(
    { width, columnGutter: 8, columnWidth: 172 },
    [cats.length]
  );

You should tell positioner about collection size changes.

May it makes sense to add items as usePositioner's dependency inside Masonic component.

@jaredLunde what do you think?

[cryptoAlgorithm]

@sebyx07 good suggestion, but I'm sure there'll be issues managing 10k+ items with a few thousand more deleted items...

@khmm12 this solution seems to be promision. I'll impliment it when I have the time. I was using a hack previously to essentially reinitialise masonic every time the length of the data changes (yes, very inefficient)

[mattandersonfg]

@khmm12 how would you solve this issue happening when a masonic <List /> is in use? usePositioner is not an option there.

[khmm12]

@mattandersonfg in this case key is rescue (with some downsides), but I would go with useMasonry.

Personally I don't see any problem to use hooks directly or create your component using these hooks. Masonry and List components do nothing special.

P.S. I don't want to be "Devil's advocate". IMO Masonry component should include items.length as usePositioner dependency, because it's some kind of "black-box" which should work out-of-box by its definition. For everything else where flexibility is needed there are hooks.

[mattandersonfg]

@khmm12 I'm providing both a key and itemKey to the List, but I need to be able to delete items from the list. On deletion there's an "Invalid value used as weak map key" error.

[rockhentai]

same problem

[gadicc]

Ugly But It Works™:

Copy of https://github.com/jaredLunde/masonic/blob/main/src/masonry.tsx with additions:

+  // Workaround for https://github.com/jaredLunde/masonic/issues/12
+  const itemCounter = React.useRef<number>(props.items.length);
+  let shrunk = false;
+  if (props.items.length !== itemCounter.current) {
+    if (props.items.length < itemCounter.current) shrunk = true;
+    itemCounter.current = props.items.length;
+  }
+
- nextProps.positioner = usePositioner(nextProps);
+ nextProps.positioner = usePositioner(nextProps, [shrunk && Math.random()]);

We only update on a smaller array because otherwise there is flicker (e.g. on more items coming in from infinite scroll). Make sure you key and itemKey are set too (as per comments above).

[kostia-official]

Will post my quick workaround for Masonry component. After detecting an item remove I increment removesCount that goes to grid key and rerenders grid.

  const itemsCount = items?.length;
  const prevItemsCount = usePrevious(itemsCount);

  const removesCount = useRef(0);

  const gridKeyPostfix = useMemo(() => {
    if (!itemsCount || !prevItemsCount) return removesCount.current;
    if (itemsCount < prevItemsCount) {
      removesCount.current += 1;
      return removesCount.current;
    }

    return removesCount.current;
  }, [itemsCount, prevItemsCount]);

      <Masonry
        key={`${gridKey}-${gridKeyPostfix}`}

[flnx]

Ugly But It Works™:

Copy of https://github.com/jaredLunde/masonic/blob/main/src/masonry.tsx with additions:

+  // Workaround for https://github.com/jaredLunde/masonic/issues/12
+  const itemCounter = React.useRef<number>(props.items.length);
+  let shrunk = false;
+  if (props.items.length !== itemCounter.current) {
+    if (props.items.length < itemCounter.current) shrunk = true;
+    itemCounter.current = props.items.length;
+  }
+
- nextProps.positioner = usePositioner(nextProps);
+ nextProps.positioner = usePositioner(nextProps, [shrunk && Math.random()]);

We only update on a smaller array because otherwise there is flicker (e.g. on more items coming in from infinite scroll). Make sure you key and itemKey are set too (as per comments above).

This helped a lot. I was trying to get around with that flickering for a while. Thanks!

[zspitz]

Ugly But It Works™:

Copy of https://github.com/jaredLunde/masonic/blob/main/src/masonry.tsx with additions:

+  // Workaround for https://github.com/jaredLunde/masonic/issues/12
+  const itemCounter = React.useRef<number>(props.items.length);
+  let shrunk = false;
+  if (props.items.length !== itemCounter.current) {
+    if (props.items.length < itemCounter.current) shrunk = true;
+    itemCounter.current = props.items.length;
+  }
+
- nextProps.positioner = usePositioner(nextProps);
+ nextProps.positioner = usePositioner(nextProps, [shrunk && Math.random()]);

@gadicc why do you need Math.random()?

const lengthRef = useRef(0) 
// ...
const positioner = usePositioner(
    { width, columnWidth, columnGutter },
    [items.length < lengthRef.current]
)
// ...
lengthRef.current = items.length

[ssonit]

Will post my quick workaround for Masonry component. After detecting an item remove I increment removesCount that goes to grid key and rerenders grid.

  const itemsCount = items?.length;
  const prevItemsCount = usePrevious(itemsCount);

  const removesCount = useRef(0);

  const gridKeyPostfix = useMemo(() => {
    if (!itemsCount || !prevItemsCount) return removesCount.current;
    if (itemsCount < prevItemsCount) {
      removesCount.current += 1;
      return removesCount.current;
    }

    return removesCount.current;
  }, [itemsCount, prevItemsCount]);

      <Masonry
        key={`${gridKey}-${gridKeyPostfix}`}

thanks so much, i fixed it