Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

First pass of typescript types for everything #3411

Merged
merged 22 commits into from
Jan 14, 2019

Conversation

glenjamin
Copy link
Collaborator

Welp, that took longer than I was expecting!

I think I've got a reasonable set of prop types for each module, but i got a bit tired of detailed checking towards the end.

I'm not sure whether the polymorphic as behaviour actually works correctly - although in theory the types I've got there should handle it.

Because of the generated types for the final component props, the error messages can be a bit wordy - but they do seem to also include the important information as well.

I've added dtslint into the build, but unfortunately this only checks the types against the simple.test.tsx file, it doesn't check the types against the actual implementation in any way.

Another mild annoyance is that although i've matched up the component filenames, the types don't work for the individual component files because of the lib vs src vs es part of the build process. In theory the files could be duplicated during the build for the exported code if this is desired.

@jquense
Copy link
Member

jquense commented Jan 13, 2019

This is awesome! Thanks a ton I'll review properly on Monday

jquense
jquense previously approved these changes Jan 14, 2019
Copy link
Member

@jquense jquense left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙏 thank you

@jquense
Copy link
Member

jquense commented Jan 14, 2019

oops can you fix the Travis CI error?

@glenjamin
Copy link
Collaborator Author

Good timing, I was just looking at that!

The failure seems to be caused by two issues, and I've just worked around it instead:

  1. <textarea> in @types/react has rows?: number, so it doesn't accept strings
  2. Either the ReplaceProps helper or the as inference logic doesn't quite seem to be working in typescript 3.1, and Cannot specify minVersion higher than 3.0 when using standalone microsoft/dtslint#176 prevents me from telling dtslint to only use 3.2 or higher.

@glenjamin
Copy link
Collaborator Author

Hrm, I think actually typescript 3.1 was correct in flagging an error here, and 3.2 not erroring wasn't correct 🤔

@glenjamin
Copy link
Collaborator Author

That appveyor error doesn't seem to be down to anything I've changed - unless I've overlooked something?

@jquense
Copy link
Member

jquense commented Jan 14, 2019

ya Appveyer is just been broken for a while. not your problem :P

@jquense jquense merged commit 2079b22 into react-bootstrap:master Jan 14, 2019
@bes bes mentioned this pull request Jan 15, 2019
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.

None yet

2 participants