yaodingyd / react-flickity-component

A React.js component for using @desandro's Flickity
314 stars 51 forks source link

reloadOnUpdate behaviour is not intuitive #147

Closed gpoole closed 5 months ago

gpoole commented 1 year ago

The behaviour of reloadOnUpdate doesn't work as I expected. The Flickity component is not memoised, which means a re-render of the parent will always cause an update in the Flickity component, regardless of whether the props changed. I think most users would interpret reloadOnUpdate to mean "reload if any of the props change", not every time the parent renders. For me, this behaviour was both confusing and also caused problems with a parent component that updates its state in response to changes in Flickity.

A potential fix is to extend PureComponent, but unfortunately it wouldn't be that simple as the children prop is never equal between renders when compared using shallowEqual, so children would need to be handled in a custom way (maybe checking for keys?). Since the fix is a little tricky and may still be confusing for users, might just be worth just documenting this problem and explaining how to avoid it? Maybe potentially just extending PureComponent and documenting the need to memoise children explicitly by users? I am happy to contribute a PR for this.

For users, you can use a workaround to get reloadOnUpdate to only trigger reloads when props passed to Flickity change:

import ReactFlickity, { FlickityOptions } from 'react-flickity-component';
import { memo, useMemo } from 'react';

// Memoise Flickity so it only re-renders when props change.
// See https://react.dev/reference/react/memo#skipping-re-rendering-when-props-are-unchanged
const MemoisedFlickity = memo(ReactFlickity);

interface Props {
  imageUrls: string[];
}

const MyComponent = ({ imageUrls }: Props) => {
  // Memoise the options prop so that it isn't a new object on every render.
  // See https://react.dev/reference/react/useMemo
  const flickityOptions = useMemo(() => ({
    /* options here */
  }, []);

  return (
    <MemoisedFlickity options={flickityOptions} reloadOnUpdate>
      {/*
        Memoise children prop being passed to Flickity so re-renders in the parent don't trigger an update unless the children actually changed.
        See https://gist.github.com/slikts/e224b924612d53c1b61f359cfb962c06
      */}
      {useMemo(() => {
        return imageUrls.map((imageUrl) => <img src={imageUrl} key={imageUrl} />);
      }, [imageUrls]);
    </MemoisedFlickity>
  );
};
yaodingyd commented 1 year ago

Good finding! care to open a PR and add this to readme? it can follow the reloadOnUpdate paragraph in "how it works" section

yaodingyd commented 5 months ago

docs updated