-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
TypeScript all the things #5150
Conversation
the export interface FormControlProps {
...
}
declare interface FormControl
extends BsPrefixRefForwardingComponent<'input', FormControlProps> {
Feedback: typeof Feedback;
}
const RBFormControl: FormControl = React.forwardRef((props: FormControlProps, ref: Ref) => {
return (...)
}) What happens is that inside the FormControl Props is just the defined FormControlProps, which is what you want, you can only rely on props you know exist. But a user of FormControl is getting the correct type that will respect their |
Cheers for the pointer. I'll look into that :) |
ebebac8
to
c847d7a
Compare
975954a
to
895e1bc
Compare
@jquense et al.: any idea why Travis isn't building anymore? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did a real quick pass and got as far as the FormCheck. Looking great! I'm not sure what is going on with travis ... @bpas247 you know?
const actual = element.style[prop]; | ||
element.dataset[prop] = actual; | ||
css(element, { | ||
[prop]: `${parseFloat(css(element, prop)) + adjust}px`, | ||
[prop]: `${parseFloat(css(element, prop as any)) + adjust}px`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the T
isn't doing a much here if we have to cast to any right below yeah?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It kind of is... T
allows us to read element.style
safely, but css(...)
uses a different typing, so... maybe some sort of intersection type between keyof CSSStyleDeclaration
and keyof CSS.Properties
?
const [direction, setDirection] = useState('next'); | ||
|
||
const [isSliding, setIsSliding] = useState(false); | ||
const [renderedActiveIndex, setRenderedActiveIndex] = useState(activeIndex); | ||
const [renderedActiveIndex, setRenderedActiveIndex] = useState<number>( | ||
activeIndex || 0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a little concerned with the coercing to 0
having unintended consequences
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess, yeah, but there are places doing arithmetic like renderedActiveIndex - 1
without checking whether it is might happen to be undefined
... What do you think, fix those to assume things or..?
{(state, innerProps) => | ||
React.cloneElement(children, { | ||
{(state, innerProps) => { | ||
return React.cloneElement(children as any, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can children be typed as React.ReactElement
to avoid this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not without refactoring Collapse into an FC. ClassComponents' props
are typed like
readonly props: Readonly<P> & Readonly<{ children?: ReactNode }>;
menuProps.alignRight = alignEnd; | ||
(menuProps as any).show = show; | ||
(menuProps as any).close = close; | ||
(menuProps as any).alignRight = alignEnd; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to cast this to a more correct type vs casting to any below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I suppose, but how do we know the props of the custom Component
? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't, but that's not important here, the Component must accept these props, e.g. the interface for whatever Component is, is "accepts at least these props". That said i think that's less important than just correctly typing menuProps so that it's safe to use here not just so Component knows it.
@jquense Looks like the last build seems to be failing due to the prettier step, although I'm not sure why it's stopped kicking off for those new commits after (also strange that GitHub actions isn't kicking off at all here). |
Update: This might be due to how many times the git history has been re-written for this PR (especially considering the last build references a commit that doesn't exist anymore), I did notice that there is a few merge conflicts so hopefully the CI builds will kick off again with a merge commit |
@bpas247 Rebasing on origin/master seemed to do the trick. :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great work! I would merge this probably soon and get rid of type any/as unknown in further iterations.
How big is the confidence in general, that this breaks existing projects @jquense / other maintainers?
Probably since mostly types were added and the tests are passing the risk is not that big in my mind. Maybe bundling / publishing / types issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking great. I think one thing we should change is probably move the bsPrefix
type from the as
component type helper and add it to each set of props manually, like we did with proptypes. That would help ensure that the type helpers don't get misused just to add the bs* props
menuProps.alignRight = alignEnd; | ||
(menuProps as any).show = show; | ||
(menuProps as any).close = close; | ||
(menuProps as any).alignRight = alignEnd; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't, but that's not important here, the Component must accept these props, e.g. the interface for whatever Component is, is "accepts at least these props". That said i think that's less important than just correctly typing menuProps so that it's safe to use here not just so Component knows it.
Component: 'figure', | ||
}); | ||
}) as unknown) as Figure; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
usually we deal with this by using Object.assign
which handles adjusting the type
type?: 'valid' | 'invalid'; | ||
} | ||
|
||
type Feedback = BsPrefixRefForwardingComponent<'div', FeedbackProps>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: it's a little confusing, maybe, to have two things e.g. both named Feedback
here? maybe we can just inline the type def?
> | ||
{children} | ||
</Component> | ||
</SelectableContext.Provider> | ||
</AccordionContext.Provider> | ||
); | ||
}); | ||
}) as unknown) as Accordion; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a better way of typing this?
Hey @akx, would be awesome if you could go over the comments if you have time, because once this is merged, we can start with making it Bootstrap 5 compatible. |
ad47589
to
4428e9d
Compare
Okay, rebased on origin/master again and upgraded some dependencies required, addressed most if not all review comments... @mxschmitt: Thank you! I also agree this should be merged sooner rather than later. There will likely be some breakage for both TS and JS consumers since the typing likely isn't 100% watertight, but it won't get fixed without merging this... Also, I think the Gatsby site build is definitely broken right now, and I don't quite have enough Gatsby-fu to look into it. @jquense: Yeah, there actually is a @taion: I was actually surprised TS allows you to name a variable the same as a type, but apparently so! Those confusions could be fixed up in a later PR, yeah? @jquense, @bpas247: Re the |
I think we should merge this into a typescript branch here, for followups and so @akx doesn't have to keep it up to date with master himself. I'm concerned that pushing this right to master will leave us in a bad spot releasing, when i'd like to test this out and do a prerelease for folks to try before we push it out to latest, given the general risk involved. also we need to fix the docs |
I changed the base! we can merge once the yarn.lock conflict is fixed |
@jquense Rebased & fixed conflicts! |
awesome work @akx Thanks a ton! |
By the way, I happened upon this TS implementation of ref forwarding: https://github.com/reach/reach-ui/blob/52d721cd643d1a6d24c7253ab24e114be262d092/packages/utils/src/index.tsx#L193-L217 Not sure if it's safer or less so than ours though :) |
* TypeScript all the things (#5150) * Add Typescript tooling via react-overlays * Add @types/ via akx/autotypes * Mostly mechanically merge JavaScript and TypeScript files * Fix typings in all TS files * BootstrapModalManager: add types + fix marging -> margin typo * Amend test and build configuration * Address some review comments * Address more review comments * Move simple-types-test into tests/ and fix things to have it typecheck * Update dev deps for e.g. Eslint 7+ compat & fix type issues * chore: fix types build output (#5204) * Publish v1.1.0-rc.0 * fix up * lint * fix defaults Co-authored-by: Aarni Koskela <akx@iki.fi>
To follow up on #5148, this PR starts to convert
src/
to TypeScript.I grabbed the TS/eslint configuration from react-overlays, loosened it up a little for development purposes. A little Python script (enclosed) merged .js and .d.ts files into .tsx files, and I then began working in the dim light of
while true; yarn tsc --noEmit 2>&1 >tmp.log; clear; head -n50 tmp.log; sleep 5; end
...23:56, 27 April
tsc
checks,as="element"
polymorphism is very likely lost on them right now. I'm having a somewhat hard time wrapping my head around the typings required to make that work in a strict enough way; it'll likely require some deeper type magic than I'm accustomed to so far! (Any ideas? 🤔)propTypes
onto these components requires an uglyas any
thing 🤢 – I expect there to be noas any
s when we're collectively finished with this...Anyway! It's a start, and judging by stricter typing finding us #5149, I think it's worth it 😄
22:59, 5 May
Okay, the test suites pass now!
as any
s,as unknown as X
s and some// @ts-ignore
s.propTypes
declarations match all*Props
interfaces.as
polymorphism is now implemented with atype ...
; I'm not sure if that's the right way, but it seems to work and type-checks alright.Card.Body
etc. feels evil (it's where I couldn't think of any other ways to do things but thatas unknown as X
thing).createWithBsPrefix
are utterly poorly typed. :(