mifi / react-lottie-player

Fully declarative React Lottie player
MIT License
505 stars 53 forks source link

Typescript: Lottie-web breaking change in 5.9.2 #70

Open altruong opened 2 years ago

altruong commented 2 years ago

Lottie-web just updated their ts typing for loadAnimation parameter in their latest version v5.9.2 https://github.com/airbnb/lottie-web/commit/eb6d9a601e8ab724a1bcc285c5efe96243f52dc6

Which breaks the current version of react-lottie-player

image

I've set my resolutions for lottie-web for v5.9.1 until its fixed, thanks!

mifi commented 2 years ago

Should this instead be fixed at lottie-web, and not here? Pardon my ignorance but I’m not an expert in ts

altruong commented 2 years ago

All good! I'm a little confused as well.

In the error that I have above, it says that in index.d.ts of this project, parameters renderer, renderingSettings and audioFactory are required but in the types from AnimationConfig, those parameters are optional so I'm not sure why TS is complaining.

declare module 'react-lottie-player' {
  import type {
    AnimationConfig,
    AnimationDirection,
    AnimationEventCallback,
    AnimationSegment
  } from 'lottie-web'

  type LottieProps = React.DetailedHTMLProps<
    React.HTMLAttributes<HTMLDivElement>,
    HTMLDivElement
  > &
    Pick<AnimationConfig, 'loop' | 'renderer' | 'rendererSettings' | 'audioFactory'> & {
      play?: boolean
      goTo?: number
      speed?: number
      direction?: AnimationDirection
      segments?: AnimationSegment | AnimationSegment[]

      onComplete?: AnimationEventCallback
      onLoopComplete?: AnimationEventCallback
      onEnterFrame?: AnimationEventCallback
      onSegmentStart?: AnimationEventCallback
    } & ({ path?: string } | { animationData?: object })

  const Lottie: React.FC<LottieProps>

  export default Lottie

To reproduce the error, you can try initialising the player in tsx where lottie-web is v5.9.2

import Lottie from 'react-lottie-player';
<Lottie
    loop={false}
    play={true}
    animationData={link_to_file}
  />
ritute commented 2 years ago

Also ran into this, so I created a wrapper specifying those now required attributes. Could be related to this which was causing a bunch of issues for us: https://github.com/facebook/react/issues/24304#issuecomment-1094565891

monsonjeremy commented 2 years ago

It seems this has to do with the fact that audioFactory was removed from the AnimationConfig type declaration. This means Pick returns it as type unknown which is not optional.

This is a workaround for now

import Lottie from 'react-lottie-player';
<Lottie
    loop={false}
    play={true}
    animationData={link_to_file}
    audioFactory={undefined}
  />
altruong commented 2 years ago

Was it removed? I still see it in index.d.ts file in the latest version of lottie-web github repo!

For reference: lottie-web v5.9.4 lottie-web v5.9.3

The only change I see is that this line (line 79 on lottie-web v5.9.3)

export type AnimationConfig<T extends 'svg' | 'canvas' | 'html' = 'svg'> = {}

was refactored to

export type RendererType = 'svg' | 'canvas' | 'html';

export type AnimationConfig<T extends RendererType> = {}

And so this line also got changed (line 101 on lottie-web v5.9.3)

export type AnimationConfigWithPath = AnimationConfig & {
    path?: string;
}

export type AnimationConfigWithData = AnimationConfig & {
    animationData?: any;
}

to

export type AnimationConfigWithPath<T extends RendererType> = AnimationConfig<T> & {
    path?: string;
}

export type AnimationConfigWithData<T extends RendererType> = AnimationConfig<T> & {
    animationData?: any;
}

It's a small change and nothing got removed (unless im blind) so not sure why TS is complaining about

monsonjeremy commented 2 years ago

@altruong This package depends on "lottie-web": "^5.7.6" which means these are the index.d.ts contents: https://github.com/airbnb/lottie-web/blob/v5.7.6/index.d.ts#L79-L88

The original opener of the ticket is correct that the types have changed, but it looks like it's not in the way they described.

The change in lottie-web version in this package is really old, but it's possible that downstream dependencies were causing the lockfiles to resolve a different version, and a change in one of the downstreams caused the package to resolve to this version. I'ts hard to debug, but it's definitely an issue related to the version resolving to 5.7.6 and the missing audioFactory type.