Skip to content

Fix Cropper defaultProps types - #323

Merged
ValentinH merged 1 commit into
ValentinH:mainfrom
cvolant:fix-cropper-default-props-types
Dec 4, 2021
Merged

Fix Cropper defaultProps types#323
ValentinH merged 1 commit into
ValentinH:mainfrom
cvolant:fix-cropper-default-props-types

Conversation

@cvolant

@cvolant cvolant commented Dec 3, 2021

Copy link
Copy Markdown
Contributor

Motivation

import dynamic from 'next/dynamic'
const Cropper = dynamic(() => import('react-easy-crop'))

Using the above Next.js dynamic import, Typescript complains that Cropper defaultProps are not assignable to Cropper props.
Why? Because defaultProps set cropShape to 'rect' and objectFit to 'contain', which is good since:

export type CropperProps = {
  // ...
  cropShape: 'rect' | 'round'
  objectFit?: 'contain' | 'horizontal-cover' | 'vertical-cover'
  // ...
}

... but doing that, TS consider that defaultProps set cropShape and objectFit to a string.

What this PR does

It merely adds an as const behind the values given to cropShape and objectFit in defaultProps.

@codesandbox-ci

codesandbox-ci Bot commented Dec 3, 2021

Copy link
Copy Markdown

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit f054854:

Sandbox Source
react-easy-crop Configuration

@raed667

raed667 commented Dec 4, 2021

Copy link
Copy Markdown
Contributor

Would using TypeScript Enums also solve the issue ?

@ValentinH

Copy link
Copy Markdown
Owner

Would using TypeScript Enums also solve the issue ?

I'm not a big fan of enums in general as they don't bring anything more than literals unions and are not supported by Babel.

I'm not sure but if we use enums for the props definition, library users will have to use the enum as well when setting the props.

I'll merge and release this PR once I'm on the computer. I also want to check what the d.ts output looks like.

@ValentinH

Copy link
Copy Markdown
Owner

All good for me 🙂

@ValentinH
ValentinH merged commit 161f652 into ValentinH:main Dec 4, 2021
@ValentinH

Copy link
Copy Markdown
Owner

Released as v4.0.1. Thank you for your contribution 🎉

@ValentinH

Copy link
Copy Markdown
Owner

@all-contributors please add @cvolant for code

@allcontributors

Copy link
Copy Markdown
Contributor

@ValentinH

I've put up a pull request to add @cvolant! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants