TypeScript all the things by akx · Pull Request #5150 · react-bootstrap/react-bootstrap

akx · 2020-04-27T21:02:40Z

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

some helper functions and such, used by multiple components, have either been strictly typed or any-typed to make things easier for the time being
a small part of components pass tsc checks,
- ⚠️ but the 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? 🤔)
- in the same vein, assigning propTypes onto these components requires an ugly as any thing 🤢 – I expect there to be no as anys 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!

There are lots of as anys, as unknown as Xs and some // @ts-ignores.
There's a handful of TODO comments, mostly for types and such that weren't quite documented.
I haven't checked whether all propTypes declarations match all *Props interfaces.
The as polymorphism is now implemented with a type ...; I'm not sure if that's the right way, but it seems to work and type-checks alright.
build seems to result in something sane, but I'm not sure if it's equivalent to what was there before.
The typing for Card.Body etc. feels evil (it's where I couldn't think of any other ways to do things but that as unknown as X thing).
Components created with createWithBsPrefix are utterly poorly typed. :(
The generated .d.ts files look pretty horrible, since they include all of react's builtin HTML element names...

jquense · 2020-04-28T13:20:04Z

the as typing is pretty tricky. The pattern i've found that works best is to not try and type as as part of the component definition, but declare it separately and cast the component to it. The Form components show the right pattern for that. Something like:

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 as prop

akx · 2020-04-28T14:27:19Z

the as typing is pretty tricky. The pattern i've found that works best is to not try and type as as part of the component definition, but declare it separately and cast the component to it. The Form components show the right pattern for that.

Cheers for the pointer. I'll look into that :)

akx · 2020-05-05T21:01:11Z

@jquense et al.: any idea why Travis isn't building anymore?

jquense

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?

jquense · 2020-05-06T13:21:42Z

src/BootstrapModalManager.tsx

    element.dataset[prop] = actual;
    css(element, {
-      [prop]: `${parseFloat(css(element, prop)) + adjust}px`,
+      [prop]: `${parseFloat(css(element, prop as any)) + adjust}px`,


the T isn't doing a much here if we have to cast to any right below yeah?

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?

src/BootstrapModalManager.tsx

jquense · 2020-05-06T13:25:54Z

src/Carousel.tsx

  const [isSliding, setIsSliding] = useState(false);
-  const [renderedActiveIndex, setRenderedActiveIndex] = useState(activeIndex);
+  const [renderedActiveIndex, setRenderedActiveIndex] = useState<number>(
+    activeIndex || 0,


a little concerned with the coercing to 0 having unintended consequences

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..?

jquense · 2020-05-06T13:27:45Z

src/Collapse.tsx

-        {(state, innerProps) =>
-          React.cloneElement(children, {
+        {(state, innerProps) => {
+          return React.cloneElement(children as any, {


can children be typed as React.ReactElement to avoid this?

Not without refactoring Collapse into an FC. ClassComponents' props are typed like

readonly props: Readonly<P> & Readonly<{ children?: ReactNode }>;

src/DropdownButton.tsx

jquense · 2020-05-06T13:31:53Z

src/DropdownMenu.tsx

-      menuProps.alignRight = alignEnd;
+      (menuProps as any).show = show;
+      (menuProps as any).close = close;
+      (menuProps as any).alignRight = alignEnd;


I think we need to cast this to a more correct type vs casting to any below

Yeah, I suppose, but how do we know the props of the custom Component? 🤔

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.

src/Feedback.tsx

bpas247 · 2020-05-06T18:48:56Z

@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).

bpas247 · 2020-05-06T19:02:38Z

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

akx · 2020-05-07T14:06:19Z

@bpas247 Rebasing on origin/master seemed to do the trick. :)

merge_ts.py

package.json

mxschmitt

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.

jquense

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

jquense · 2020-05-13T14:01:03Z

src/DropdownMenu.tsx

-      menuProps.alignRight = alignEnd;
+      (menuProps as any).show = show;
+      (menuProps as any).close = close;
+      (menuProps as any).alignRight = alignEnd;


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.

src/DropdownMenu.tsx

src/DropdownItem.tsx

src/DropdownButton.tsx

src/DropdownItem.tsx

src/DropdownToggle.tsx

jquense · 2020-05-13T14:09:37Z

src/Figure.tsx

+const Figure: Figure = (createWithBsPrefix('figure', {
  Component: 'figure',
-});
+}) as unknown) as Figure;


usually we deal with this by using Object.assign which handles adjusting the type

merge_ts.py

simple-types-test.tsx

taion · 2020-05-14T03:01:54Z

src/Feedback.tsx

+  type?: 'valid' | 'invalid';
+}
+
+type Feedback = BsPrefixRefForwardingComponent<'div', FeedbackProps>;


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?

src/NavbarCollapse.tsx

src/Switch.tsx

src/AbstractNav.tsx

bpas247 · 2020-05-14T03:19:16Z

src/Accordion.tsx

    </AccordionContext.Provider>
  );
-});
+}) as unknown) as Accordion;


Is there a better way of typing this?

src/AccordionContext.tsx

src/Accordion.tsx

mxschmitt · 2020-05-16T17:18:37Z

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.

akx · 2020-05-17T15:16:52Z

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 BsPrefixAndClassNameOnlyProps type now, but that's one unwieldy name! :)

@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 as unknown as any hacks, I think those could be replaced with Object.assigns in a subsequent PR.

jquense · 2020-05-18T17:49:02Z

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

jquense · 2020-05-18T17:50:20Z

I changed the base! we can merge once the yarn.lock conflict is fixed

akx · 2020-05-19T11:09:59Z

I changed the base! we can merge once the yarn.lock conflict is fixed

@jquense Rebased & fixed conflicts!

jquense · 2020-05-19T12:38:08Z

awesome work @akx Thanks a ton!

akx · 2020-05-19T12:57:09Z

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>

akx force-pushed the all-ts branch 3 times, most recently from ebebac8 to c847d7a Compare May 5, 2020 19:58

akx changed the title ~~Heavily WIP: TypeScript all the things~~ TypeScript all the things May 5, 2020

akx force-pushed the all-ts branch 3 times, most recently from 975954a to 895e1bc Compare May 5, 2020 20:56

akx marked this pull request as ready for review May 5, 2020 21:01

jquense reviewed May 6, 2020

View reviewed changes

akx force-pushed the all-ts branch from 895e1bc to 6cbd930 Compare May 7, 2020 14:05

mxschmitt mentioned this pull request May 12, 2020

Bootstrap 5 support #5174

Closed

mxschmitt requested a review from jquense May 12, 2020 21:02

mxschmitt reviewed May 12, 2020

View reviewed changes

merge_ts.py Outdated Show resolved Hide resolved

mxschmitt reviewed May 12, 2020

View reviewed changes

package.json Outdated Show resolved Hide resolved

mxschmitt approved these changes May 12, 2020

View reviewed changes

jquense reviewed May 13, 2020

View reviewed changes

taion reviewed May 14, 2020

View reviewed changes

bpas247 reviewed May 14, 2020

View reviewed changes

akx force-pushed the all-ts branch 3 times, most recently from ad47589 to 4428e9d Compare May 17, 2020 15:14

akx force-pushed the all-ts branch from 4428e9d to 3e68830 Compare May 17, 2020 16:00

jquense changed the base branch from master to typescript May 18, 2020 17:49

akx added 10 commits May 19, 2020 14:05

Add Typescript tooling via react-overlays

09941da

Add @types/ via akx/autotypes

fc03f1d

Mostly mechanically merge JavaScript and TypeScript files

22bffe7

BootstrapModalManager: add types + fix marging -> margin typo

e782b41

Fix typings in all TS files

0dfab2a

Amend test and build configuration

dc66899

Address some review comments

1907f06

Address more review comments

84eabec

Move simple-types-test into tests/ and fix things to have it typecheck

ec49309

Update dev deps for e.g. Eslint 7+ compat & fix type issues

e2f05cc

akx force-pushed the all-ts branch from 3e68830 to e2f05cc Compare May 19, 2020 11:06

jquense merged commit 179fec9 into react-bootstrap:typescript May 19, 2020

akx mentioned this pull request May 27, 2020

All-in TypeScript? #5148

Closed

Conversation

akx commented Apr 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

23:56, 27 April

22:59, 5 May

Uh oh!

jquense commented Apr 28, 2020

Uh oh!

akx commented Apr 28, 2020

Uh oh!

akx commented May 5, 2020

Uh oh!

jquense left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bpas247 commented May 6, 2020

Uh oh!

bpas247 commented May 6, 2020

Uh oh!

akx commented May 7, 2020

Uh oh!

Uh oh!

Uh oh!

mxschmitt left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jquense left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mxschmitt commented May 16, 2020

Uh oh!

akx commented May 17, 2020

Uh oh!

jquense commented May 18, 2020

Uh oh!

jquense commented May 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

akx commented Apr 27, 2020 •

edited

Loading

mxschmitt left a comment •

edited

Loading

jquense commented May 18, 2020 •

edited

Loading