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

Mantine Carousel onSlideChange method become stale closure #6393

Open
1 of 2 tasks
leadq opened this issue Jun 15, 2024 · 0 comments
Open
1 of 2 tasks

Mantine Carousel onSlideChange method become stale closure #6393

leadq opened this issue Jun 15, 2024 · 0 comments
Labels
Fixed patch Completed issues that will be published with next patch (1.0.X)

Comments

@leadq
Copy link

leadq commented Jun 15, 2024

Dependencies check up

  • I have verified that I use latest version of all @mantine/* packages

What version of @mantine/* packages do you have in package.json?

7.10.2

What package has an issue?

@mantine/carousel

What framework do you use?

Next.js

In which browsers you can reproduce the issue?

Chrome

Describe the bug

I am using the Carousel component. The onSlideChange method of the Carousel becomes stale due to the implementation within the library. When I examined the source code, I understood the reason, but I couldn't run the source code in a dev environment. That's why I'm opening this issue.

Here is the method I provide as a callback:

 const handleSlideChange = (index: number) => {
    if (pages) { // if is unnecessary for this issue
      const currentPage = pages[index];
      const previousPage = pages[slideIndex]; // slideIndex is a state

      if (index + 1 === totalPageCount && !isReachedEnd) { // isReachedEnd is a state
        setIsReachedEnd(true);
        onReachEnd?.(currentPage, index); // callback inside parent component
      }
      setSlideIndex(index);

      if (currentPage) {
        onPageChange(currentPage, index, previousPage);
      }
    }
  };

As you can see, I am trying to call the onPageChange method in the parent with the previous page. However, due to the stale closure issue inside Carousel.tsx, the "slideIndex" within the handler never sees the current value(it always sees the value which comes at very first time). But I confirm that slideIndex in my component is set and changes everytime.

Another example is the usage of isReachedEnd state. If I reach the last slide, I want to call the onReachedEnd method in the parent component, and I want it to happen only once. That's why I wrote an if statement: if isReachedEnd is false, set it to true and notify the parent. Unfortunately, this also doesn't work because, due to the stale closure issue, the method keeps reading the "false" value from the initial render. My expectation is I should not have to define a state then use "useEffect" to handle my business.

The reason is this useEffect and handleSelect method in mantine/Carousel.tsx:

// Carousel.tsx
 const handleSelect = useCallback(() => {
    if (!embla) return;
    const slide = embla.selectedScrollSnap();
    setSelected(slide);
    onSlideChange?.(slide);
  }, [embla, setSelected]); // =>> here is the issue, onSlideChange isn't covered in deps array

and the useEffect

// Carousel.tsx
 useEffect(() => {
    if (embla) {
      getEmblaApi?.(embla);
      handleSelect();
      setSlidesCount(embla.scrollSnapList().length);
      embla.on('select', handleSelect);

      return () => {
        embla.off('select', handleSelect);
      };
    }

    return undefined;
  }, [embla, slidesToScroll]); // ==>> handleSelect isnt covered in deps array

Because it is not included in the dependency arrays, the callback is cached as it was first passed, and I can never create the updated callback in the current render.

This is the cause of the issue, but I'm not sure how to fix it in mantine Carousel.tsx because I couldn't run in dev mode.
However, after making these adjustments in the source code within the node_modules, I saw that the stale closure problem was resolved. But this time, I noticed that the render was triggered twice, which led to another issue.

I will read contributor article very soon, but I dont have so much time because I have freelence work, so If I can do before you, I will

If possible, include a link to a codesandbox with a minimal reproduction

https://codesandbox.io/p/sandbox/mantine-react-template-forked-ptfj7y?file=%2Fsrc%2FChild.tsx%3A49%2C37

Possible fix

No response

Self-service

  • I would be willing to implement a fix for this issue
@rtivital rtivital added the Fixed patch Completed issues that will be published with next patch (1.0.X) label Jun 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fixed patch Completed issues that will be published with next patch (1.0.X)
Projects
None yet
Development

No branches or pull requests

2 participants