react-intersection-observer icon indicating copy to clipboard operation
react-intersection-observer copied to clipboard

`InView` component types are not compatible with the `@emotion/react`

Open faminchik opened this issue 3 years ago • 4 comments
trafficstars

Describe the bug

@emotion/react library allows to pass the css property into components if the className prop is a part of them. The InView has the className prop but typescript complains when passing the css prop.

@emotion/react types inject this property using the following type:

type WithConditionalCSSProp<P> = 'className' extends keyof P
  ? string extends P['className' & keyof P]
    ? { css?: Interpolation<Theme> }
    : {}
  : {}

But since the InView component accepts props with the IntersectionObserverProps | PlainChildrenProps union type, the mentioned WithConditionalCSSProp type returns {} since the className props exists only in the PlainChildrenProps type.

To Reproduce

CodeSandbox

Expected behavior InView component types should be compatible with the @emotion/react

faminchik avatar Jul 26 '22 10:07 faminchik

Might you be able to solve this using a type guard?

Otherwise, how do you imagine the types being changed to support this pattern?

thebuilder avatar Jul 26 '22 21:07 thebuilder

@thebuilder,

I've tried to play with types of this component and have noticed that if using children as a function like described in the IntersectionObserverProps type [children: (fields: RenderProps) => React.ReactNode], typescript doesn't complain if I try passing the className property but it should complain because this type (IntersectionObserverProps) doesn't have this prop actually and the class attribute itself doesn't appear in the DOM. (it's possible to use the CodeSandox from the initial message to take a look at this scenario)

The solution (without deep investigation, to be honest) to resolve the initial issue and make the InView component types a bit more flexible might be the following:

(In this file)

- export class InView extends React.Component<IntersectionObserverProps | PlainChildrenProps, State>
+ export class InView<T extends IntersectionObserverProps | PlainChildrenProps> extends React.Component<T, State>
- constructor(props: IntersectionObserverProps | PlainChildrenProps);
+ constructor(props: T);

At least with these additions, there will be a possibility to specify the desired type for component props what makes it more strict in the current situation, I would say

faminchik avatar Jul 27 '22 10:07 faminchik

Suppose it could make sense to make some better type inference based on the type of children. It might also be solved if the type for PlainChildrenProps extends a React.Component, instead of just HTMLProps. Kinda depends on where Emotion injects the css prop type.

Any reason you can't use the render props pattern to solve this issue?

thebuilder avatar Jul 27 '22 12:07 thebuilder

Actually, it seems the render props pattern may help, yes.

Anyway, hope this discussion was useful for possible future improvements 😌

faminchik avatar Jul 27 '22 13:07 faminchik