davidhu2000 / react-spinners

A collection of loading spinner components for react
https://www.davidhu.io/react-spinners
MIT License
3.11k stars 267 forks source link

PropTypes validation error on css prop #62

Closed RobertDoc closed 5 years ago

RobertDoc commented 5 years ago

Do you want to request a feature or report a bug? Bug

If reporting a bug, what version of react-spinners are you currently using?

 "react-spinners": {
            "version": "0.5.4",
            "resolved": "https://registry.npmjs.org/react-spinners/-/react-spinners-0.5.4.tgz",
            "integrity": "sha512-jo7BE8prvnZNL7xNrQL16geVXH6LmaWrhvvXnmUwz2MhFO14bhlAdCLuKCwqmUJ37kGphH0C2CJRThwkjfpVzw==",
            "requires": {
                "@emotion/core": "^10.0.4",
                "prop-types": "^15.5.10",
                "recompose": "0.27.1 - 0.30.0"
            }
        },

What is the current behavior? When using TypeScript & Emotion, the following will compile correctly:

 <FadeLoader
                        css={`display: block; margin: 0 auto;`}
                        color={"#000000"}
                        loading={true}
                    />

However this triggers a PropTypes validation error:

Warning: Failed prop type: Invalid prop css of type string supplied to Loader, expected object.

If the current behavior is a bug, please provide the steps to reproduce

 <FadeLoader
                        css={`display: block; margin: 0 auto;`}
                        color={"#000000"}
                        loading={true}
                    />
davidhu2000 commented 5 years ago

Hi @RobertDoc, looks like you are not using the css function from emotion?

import { css } from '@emotion/core';
<FadeLoader
  css={css`display: block; margin: 0 auto;`}
  color={"#000000"}
  loading={true}
/>
danielcranford commented 5 years ago

Hi @davidhu2000, The problem with using css from emotion with typescript is that react-spinners has an incorrect typescript definition of the type of the css attribute, leading to compile time errors for anything except strings. eg

<FadeLoader
  css={css`display: block; margin: 0 auto;`}
  color={"#000000"}
  loading={true}
/>

causes this typescript error during compilation

TypeScript error: Type 'SerializedStyles' is not assignable to type 'string | Keyframes | (ComponentSelector & string) | (SerializedStyles & string) | (ArrayInterpolation<undefined> & string) | (ObjectInterpolation<undefined> & string) | (((theme: any) => Interpolation<...>) & string)'.
  Type 'SerializedStyles' is not assignable to type 'ObjectInterpolation<undefined> & string'.
    Type 'SerializedStyles' is not assignable to type 'ObjectInterpolation<undefined>'.
      Index signature is missing in type 'SerializedStyles'.  TS2322

    12 |      <FadeLoader
  > 13 |         css={css`display: block; margin: 0 auto;`}
       |         ^
    14 |         color={"#000000"}
    15 |         loading={true}
    16 |       />

which is caused by this interface in react-spinner's index.d.ts

    interface CommonProps {
        color?: string;
        loading?: boolean;
        css?: string;
    }

Note that the css property is declared to be a string, which is different than the react propTypes declaration

  css: PropTypes.shape({
    name: PropTypes.string,
    styles: PropTypes.string
  })

Ultimately, neither of them are completely correct, as react-spinners just forwards props.css to emotion's css template tag function as an embedded expression.

$ grep 'props\.css' -r src
src/RiseLoader.jsx:    wrapper = () => this.props.css || '';
src/PropagateLoader.jsx:    return this.props.css ? css`${wrapper};${this.props.css}` : wrapper;
src/BeatLoader.jsx:    wrapper = () => this.props.css || '';
src/BarLoader.jsx:      return this.props.css ? css`${wrapper};${this.props.css}` : wrapper;
src/SquareLoader.jsx:      return this.props.css ? css`${style};${this.props.css}` : style;
src/FadeLoader.jsx:    return this.props.css ? css`${wrapper};${this.props.css}` : wrapper;
src/HashLoader.jsx:      return this.props.css ? css`${wrapper};${this.props.css}` : wrapper;
src/SkewLoader.jsx:      return this.props.css ? css`${style};${this.props.css}` : style;
src/DotLoader.jsx:      return this.props.css ? css`${wrapper};${this.props.css}` : wrapper;
src/CircleLoader.jsx:      return this.props.css ? css`${wrapper};${this.props.css}` : wrapper;
src/ClipLoader.jsx:      return this.props.css ? css`${style};${this.props.css}` : style;
src/ScaleLoader.jsx:    wrapper = () => this.props.css || '';
src/MoonLoader.jsx:      return this.props.css ? css`${wrapper};${this.props.css}` : wrapper;
src/RotateLoader.jsx:    return this.props.css ? css`${wrapper};${this.props.css}` : wrapper;
src/RingLoader.jsx:      return this.props.css ? css`${wrapper};${this.props.css}` : wrapper;
src/PulseLoader.jsx:    wrapper = () => this.props.css || '';
src/BounceLoader.jsx:      return this.props.css ? css`${wrapper};${this.props.css}` : wrapper;
src/PacmanLoader.jsx:      return this.props.css ? css`${wrapper};${this.props.css}` : wrapper;
src/SyncLoader.jsx:    wrapper = () => this.props.css || '';
src/GridLoader.jsx:      return this.props.css ? css`${wrapper};${this.props.css}` : wrapper;
src/ClimbingBoxLoader.jsx:    return this.props.css ? css`${container};${this.props.css}` : container;

emotion's css accepts template expressions that are strings, the result of another css tagged template call, and even embedded functions although their documentation on composition is a little lacking in details.

davidhu2000 commented 5 years ago

I think the issue might be here, in the propType helper function.

const common = {
  css: PropTypes.shape({
    name: PropTypes.string,
    styles: PropTypes.string
  })
};

Looking at each individual loader, the css prop should be a string, as seen here

src/ClimbingBoxLoader.jsx:    return this.props.css ? css`${container};${this.props.css}` : container;

The css function is called on the prop css.

I think change should fix this issue

const common = {
  css: PropTypes.string
};
danielcranford commented 5 years ago

Looking at each individual loader, the css prop should be a string, as seen here

src/ClimbingBoxLoader.jsx:    return this.props.css ? css`${container};${this.props.css}` : container;

Not quite. The css prop should be whatever css from emotion accepts. They accept both string and precompiled css blocks for starters. For example, the following works

const base = css`color: hotpink;`;
const derived=css`${base};background-color: #eee;`;

base is not a string, it is an object with the shape {name: string, styles: string} see the emotion composition docs here

I think change should fix this issue

const common = {
  css: PropTypes.string
};

The react type validators are advisory. A failed typecheck will only issue a warning, and only in development mode.

The typescript types are required (although technically typescript can also be configured to be used in an advisory maner). create-react-app will fail to launch if the typescript compile fails a type check.

So you really need to change the typescript type, not just the react type. Changing both of them to be consistent would be best.

Typescript type

interface PrecompiledCss {
  name: string;
  styles: string;
}

interface CommonProps {
  color?: string;
  loading?: boolean;
  /* css can be either string, or precompiled css block */
  css?: string | PrecompiledCss;
}

React PropTypes

const precompiledCssType = PropTypes.shape({
  name: PropTypes.string,
  styles: PropTypes.string
});

const common = {
  /* css can be either string, or precompiled css block */
  css: PropTypes.oneOfType([
    PropTypes.string,
    precompiledCssType
  ])
}
davidhu2000 commented 5 years ago

thank you for the explanation @danielcranford. I've updated this package to include your suggested changes. Please try v0.5.5 and see if this issue has been resolved.

davidhu2000 commented 5 years ago

closing this issue now. Please let me know if this issue persists.