Embla-carousel: Embla ref is not initialized correctly if component returns null at the first render

[olushchik]

Hi, David,

I'm trying to migrate my components from old v2.9 with class setup to the latest version of Embla with hook and I ran into the issue which described in the title. Please see the demonstration. Is it a bug or expected behavior?

[davidcetinkaya]

Hello Oleg @olushchik,

Thank you for your question and I appreciate the CodeSandbox demonstrating the issue clearly. I would say that this is expected with the current state of the code and how the container is setup with a ref. But I also agree that it would be nice if the carousel reacted to when its container changes.

There are probably a couple of different ways to achieve this by refactoring the react hook. One idea is to convert the container ref into a state. This would add another render when the container changes but solve the issue you describe, and the code would look something along these lines:

function useEmblaCarousel(
  options?: EmblaOptions,
): [FC<PropType>, EmblaCarouselType?] {
  const [embla, setEmbla] = useState<EmblaCarouselType>()
  // const container = createRef<HTMLElement>()
  const [container, setContainer] = useState<HTMLElement>()

  useEffect(() => {
    // if (canUseDOM && container?.current) {
    if (canUseDOM && container) {
      // setEmbla(EmblaCarousel(container.current, options))
      setEmbla(EmblaCarousel(container, options))
    }
  }, [container, options])

  useEffect(() => {
    return () => embla?.destroy()
  }, [])

  const Carousel: FC<PropType> = useCallback(
    ({ htmlTagName = 'div', className, children }) => {
      return createElement(
        htmlTagName,
        {
          className,
          // ref: container,
          ref: setContainer,
          style: { overflow: 'hidden' },
        },
        children,
      )
    },
    [],
  )

  return [Carousel, embla]
}

Another idea is to create a callback ref as described here.

I think both should work but haven't investigated it much at this point. What are your thoughts?

Kindly
David

[olushchik]

I prefer any way that solves this problem :)

[davidcetinkaya]

Ok Oleg (@olushchik) 😄, I'll look into this as soon as possible then.
In the meantime, you can render an empty carousel instead of null (I know this isn't optimal). Thank you for your patience.

Best
David

[davidcetinkaya]

Hi Oleg (@olushchik),

I've created a CodeSandbox demonstration with the solution I mentioned above. Please take some time to test this with different scenarios. We wouldn't want the container to render causing the carousel to reinitialize (and stopping from scrolling) at undesired situations. Let me know how it goes!

@xiel, please feel free to join this conversation and share your thoughts if you want.

Best
David

[xiel]

Hey @davidcetinkaya ,

looks like a reasonable change. But in the code above I think you would have a memory leak in the situation when the container gets mounted and unmounted and mounted again.

The EmblaCarousel factory would be called multiple times without calling destroy beforehand.

function useEmblaCarousel(
  options?: EmblaOptions,
): [FC<PropType>, EmblaCarouselType?] {
  const [embla, setEmbla] = useState<EmblaCarouselType>()
  const [container, setContainer] = useState<HTMLElement>()

  useEffect(() => {
    if (canUseDOM && container) {
      setEmbla(EmblaCarousel(container, options))
    }
    // always cleanup, before creating a new instance
    return () => embla?.destroy()
  }, [container, options])

  const Carousel: FC<PropType> = useCallback(
    ({ htmlTagName = 'div', className, children }) => {
      return createElement(
        htmlTagName,
        {
          className,
          ref: setContainer,
          style: { overflow: 'hidden' },
        },
        children,
      )
    },
    [],
  )

  return [Carousel, embla]
}

I just noticed there also was a memory leak before, when a new options object was passed to the hook (eg. inline object).

const [EmblaCarouselReact, embla] = useEmblaCarousel({ loop: false })

This would basically create a new embla instance on each render, or do you deduplicate instances inside of Embla based on the dom node?

– Felix

[olushchik]

Hi, David,

Looks perfect! I will wait for a new version with this fix :) Thanks!

[davidcetinkaya]

Thanks a lot for sharing your thoughts Felix @xiel. I agree that both things you mention need attention 👍🏻.

Regarding the options property with the inline object being passed, I’m thinking that a shallow object comparison could solve this (the Embla options object is flat), so we could store options in a ref and only reinit Embla when options actually change.

What’s your thoughts about this suggestion?

Best
David

[olushchik]

Hi, David,

When can we expect a new version with this fix? :)

[davidcetinkaya]

Hello Oleg (@olushchik),

I'm working on this and making progress. But I wouldn't want to rush this because that will probably introduce unwanted bugs. I'll let you know as soon as I have something. Thank you for your patience.

Kindly
David

[davidcetinkaya]

So here's the current draft @olushchik, @xiel. Please feel free to share any thoughts or feedback if you want.

function useEmblaCarousel(
  options: EmblaOptions = {},
): [FC<PropType>, EmblaCarouselType?] {
  const [embla, setEmbla] = useState<EmblaCarouselType>()
  const [container, setContainer] = useState<HTMLElement>()
  const storedOptions = useRef<EmblaOptions>(options)

  const hasOptionsChanged = useCallback(
    (options: EmblaOptions) => {
      if (!areEqualShallow(storedOptions.current, options)) {
        storedOptions.current = options
      }
      return storedOptions.current
    },
    [options],
  )

  useEffect(() => {
    if (canUseDOM() && container) {
      embla?.destroy()
      setEmbla(EmblaCarousel(container, options))
    }
    return () => embla?.destroy()
  }, [container, hasOptionsChanged(options)])

  const Carousel: FC<PropType> = useCallback(
    ({ htmlTagName = 'div', className, children }) => {
      return createElement(
        htmlTagName,
        {
          className,
          ref: setContainer,
          style: { overflow: 'hidden' },
        },
        children,
      )
    },
    [],
  )

  return [Carousel, embla]
}

Best
David

[xiel]

Few minor things:

1. shadowing of options variable in hasOptionsChanged & use of complex expressions is discouraged in deps array [..., hasOptionsChanged(options)]
2. embla state is not reset to undefined when it is not available anymore (eg. container unmounted)
3. duplication - no need to call destroy before set (cleanup is always called first anyway)

I can also recommend using eslint rule of hooks, which helps to catch that all dependency array are up-to-date.

Here is a in my view corrected version:

  const [embla, setEmbla] = useState<EmblaCarouselType>()
  const [container, setContainer] = useState<HTMLElement>()
  const storedOptions = useRef<EmblaOptions>(options)
  const activeOptions = useMemo<EmblaOptions>(() => {
    if (!areEqualShallow(storedOptions.current, options)) {
      storedOptions.current = options
    }
    return storedOptions.current
  }, [storedOptions, options])

  useEffect(() => {
    if (canUseDOM() && container) {
      const newEmbla = EmblaCarousel(container, activeOptions)
      setEmbla(newEmbla)
      return () => newEmbla.destroy()
    } else {
      setEmbla(undefined)
    }
  }, [container, activeOptions])

Cheers,
– Felix

[davidcetinkaya]

Hello Felix (@xiel),
Thanks a bunch for taking the time to share your feedback. Regarding the list:

1. You're right! I rushed it a bit and the useMemo as you suggest is a better solution.
2. Nice catch. Rushed it here too. Setting this to undefined will remove the need of the duplicate destroy.
3. Point 2 solves this.

Again, thanks a lot!

[davidcetinkaya]

Hello Oleg (@olushchik),

Please read and pay attention to the migration guide here in order to update to the latest version of Embla Carousel. The hook should now work as expected and react to when its container and options object changes.

Let me know if this helps.

Cheers
David

[davidcetinkaya]

Hello Oleg (@olushchik),

Have you had a chance to try the new version out yet?

Best
David

[olushchik]

Hi, David,

Yes, I updated our project to new version and all looks good 👍 Thanks for your work!

Oleg

[davidcetinkaya]

Glad to hear that it’s working. And thank you for the feedback. Most of the cred goes to @xiel for his contributions.

Enjoy!
David