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

reloadOnUpdate behaviour is not intuitive #147

Closed
gpoole opened this issue Aug 16, 2023 · 2 comments
Closed

reloadOnUpdate behaviour is not intuitive #147

gpoole opened this issue Aug 16, 2023 · 2 comments

Comments

@gpoole
Copy link
Contributor

gpoole commented Aug 16, 2023

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>
  );
};
@gpoole gpoole mentioned this issue Aug 16, 2023
@yaodingyd
Copy link
Owner

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

@yaodingyd
Copy link
Owner

docs updated

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

No branches or pull requests

2 participants