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

fix(FormControl): onChange and innerRef ts decls (#3583,#3568,#2781) #4435

Merged
merged 1 commit into from
Oct 18, 2019

Conversation

craigpg
Copy link
Contributor

@craigpg craigpg commented Sep 9, 2019

These changes seemed get the types sorted out for my project. I added some tests to simple.test.tsx to confirm and demonstrate my usage.

May want to consider adding a test-types script to package.json and including it in the main test script.

"test": "npm run lint && npm run dtslint && npm run test-browser && npm run test-node && npm run test-types"
"test-types": "tsc --noEmit -p types"

@taion
Copy link
Member

taion commented Sep 9, 2019

we do test the types, i thought?

we should get rid of the last few innerRef instances

@craigpg
Copy link
Contributor Author

craigpg commented Sep 12, 2019

@taion - the dtslint script covers type checking just fine so my comments re: script changes can be ignored. I assumed I was missing something so I didn't include script changes in the PR.

My original PR was going to just fix the onChange type, but I noticed that innerRef was also incorrect so I fixed that while I was in there. On closer look, it doesn't look like FormControl makes use of innerRef so it can be removed. It can probably be removed from the other Form... type declarations as well. I'll update the PR if you agree.

Not sure there's a way to get clean typings for ref since React will insist on creating a ref for the wrapping component and combine that type with the inner ref type. This just means that when creating the ref, you'll need do something like this:

React.createRef<FormControl<"select"> & HTMLSelectElement>()

@taion
Copy link
Member

taion commented Sep 12, 2019

That seems weird. So is there no way to get React.createRef to have the correct types?

Copy link
Member

@taion taion left a comment

Choose a reason for hiding this comment

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

well, for the time being this is just an improvement, anyway

@craigpg
Copy link
Contributor Author

craigpg commented Sep 13, 2019

@taion - The ref types are coming from:

  1. The generic class that FormControl extends to get the props of the wrapped input element. This will add a ref prop with type HTMLInputElement, which is really the only ref we want.
  2. When TS transforms the JSX, it creates a FormControl element and the underlying TS declarations automatically add a ref prop with type FormControl by intersecting it with the rest of the props.
  3. Since the ref prop already exists, albeit with a different type, the prop types are intersected.

There is a workaround

@taion
Copy link
Member

taion commented Sep 13, 2019

thanks for digging that up.

i'm comfortable moving forward with this. we'll likely want to switch everything to forwardRef just for non-TS API concerns before we cut 1.0.0 final, but this is a strict improvement as things are now

@bpas247 bpas247 added the types label Oct 18, 2019
Copy link
Member

@bpas247 bpas247 left a comment

Choose a reason for hiding this comment

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

Definitely an improvement for the time being, great work 👍

@jquense jquense merged commit 74a36bc into react-bootstrap:master Oct 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants