Skip to content

Conversation

@illBeRoy
Copy link
Contributor

According to the API, all but the src prop are optional.

That said, the Props interface had no optional properties, making anyone who uses typescript have to pass everything (or it would throw a type error and will not compile).

I updated the interface to make the denote the non-required props as optional :)

According to the API, all but the `src` prop are optional, but the `Props` interface had no optional properties, making anyone who uses typescript **have** to pass everything (or it would throw a type error and will not compile).
I updated the interface to make the denote the non-required props as optional :)
I don't usually use the strict null check option, so wasn't expecting that ;)
@tanem
Copy link
Owner

tanem commented Nov 24, 2018

Thanks for the proposal @illBeRoy.

Can I ask what version of TypeScript you're using? I know in v3 of TS this shouldn't be an issue, but maybe there's a case for merging your PR in order to provide support for older versions of TS 🤔

@illBeRoy
Copy link
Contributor Author

illBeRoy commented Nov 24, 2018

Edit: Just now I've seen what you meant (regarding default props). This is new to me, was not aware of that. Anyways, it might be better to explicitly denote these props as optional in the interface, for the benefits of older projects using version 2 🤠

Hey again!
I'm using TS 3.1.3. But as far as I know, this behavior is per spec regardless of the compiler version 😊

The thing with React props validation in typescript, is that they make you pass all required props:
image

So if, for instance, fallback is an optional prop (according to the docs), then it should have the optional operator (?).

Hope I helped! 🙏

@tanem
Copy link
Owner

tanem commented Nov 24, 2018

Cool, I can see a similar approach is used in other popular TS libs like formik so I'll merge this PR. Just updating the examples to cover the different TS versions. Thanks for your contribution @illBeRoy! 🙏

@tanem tanem merged commit 2927a41 into tanem:master Nov 24, 2018
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.

2 participants