Convert React-Table to TypeScript

[priley86]

• convert all react-table source to tsx
• bring the reactabular-table source in house and convert it to tsx. This follows along with PF4 Bring Reactabular in-house #1628 which I have already prototyped alongside W.I.P. React virtualized extension #1250, but I will pull that change out specifically alongside tsx conversion for now.

[priley86]

• noted will need to be able to share tsconfigs/base tslint configs with other packages outside of react-core to accomplish this.

[ssilvert]

Hi @priley86, do you have an ETA for this?

We are trying to use react-table with TypeScript and we get errors about declaration files. Also looks like you need to use the latest version of react-core.

node_modules/@patternfly/react-table/dist/js/components/Table/index.d.ts(20,49): error TS7016: Could not find a declaration file for module './ExpandableRowContent'. 'C:/GitHub/keycloak/themes/src/main/resources/theme/keycloak-preview/account/resources/node_modules/@patternfly/react-table/dist/js/components/Table/ExpandableRowContent.js' implicitly has an 'any' type.
node_modules/@patternfly/react-table/node_modules/@patternfly/react-core/dist/js/components/Select/Select.d.ts(10,18): error TS2430: Interface 'SelectProps' incorrectly extends interface 'HTMLProps'.
Types of property 'onSelect' are incompatible.
Type '(event: any, element: string, isPlaceholder: boolean) => void' is not assignable to type '(event: SyntheticEvent<HTMLOptionElement, Event>) => void'.
node_modules/@patternfly/react-table/node_modules/@patternfly/react-core/dist/js/components/Select/Select.d.ts(14,19): error TS2304: Cannot find name 'ReactEventHandler'.

[priley86]

hi @ssilvert 👋

i'm guessing we'll be able to introduce this in the next few sprints - currently still trying to resolve a few other things downstream on my side. I'll probably be able to attempt this next week though. As far as I can tell, we'll need Dropdown and a few other react-core components converted to TS first. Noted that when starting this here. If you need this sooner and would like to try and contribute it (or you have a short term workaround), happy to review this w/ you.

cc: @LHinson @dgutride @rachael-phillips

[ssilvert]

Hi @priley86,

I created a reproducer for you. Just grab this branch of my simple patternfly/systemjs app. If you run 'npm run build' you will see the TypeScript errors.

If this is going to take much longer, can we start thinking about a temporary solution where we just provide some simple .d.ts files? We do need to get past this soon and I'm not 100% sure that the issue with the UMD distribution #1966 is really fixed. We haven't been able to test it.

Thanks.

cc: @dgutride @pedroigor

[priley86]

hi @ssilvert, thanks for following up w/ this (it gives a better glimpse of what you are looking at).

Regarding the typescript definitions to react-table, I've added an initial draft PR that will convert some existing react-table components to typescript and will add d.ts files for components not yet converted. This would enable you to compile react-table w/ strict mode and noImplicitAny set true. Regarding these options though, I find your current TS build configuration a bit intriguing. Currently you'd get build errors in react-core and react-table if you set strictNullChecks to true (like you have here), so you'd probably want to disable that for now. I'd also add, if PF really wants to add more strict support down the road, we should look to enabling the no-any rule in tslint. We can still compile in strict mode without this, but to be honest, what's the point of using any so frequently to avoid typing? We have over 200+ references and counting... some good/some bad uses of any it seems. More or less I'm asking, what do these build flags accomplish for your product?

This is a large effort and has certainly been a background task for me, and i'll likely not be able to convert all components until late July timeframe due to other efforts. We'd also need to test this conversion in multiple products now consuming react-table (as well as changes to react-core). You can certainly tweak this PR or consume a personal fork of this in the interim. If you need a faster response, maybe you can ask someone working actively in the PF Dev Squad? I'm happy to help fully test this later on once we can integrate in products.

Regarding UMD/this build though, I believe your build configuration is quite different from many products I've worked w/ recently using Webpack. I don't know that many teams are using UMD modules (most have moved to ESM), but certainly appreciate you helping us add that support. I'm not working on a UMD based project currently and would probably struggle to help you maintain this, but you might consider contributing your SystemJS project to PatternFly in some way? This also might be a good candidate for the patternfly-react-seed. cc: @seanforyou23

HTH!

[mturley]

+1 for adding no-any, if we really need any in a particular case we can use // tslint:disable-line:no-any on those lines.

[ssilvert]

We have used this build config from the beginning and we don't have any problems with react-core.

I want my code to produce as few runtime errors as possible. So it is advisable to keep strictNullChecks and noImplicitAny. We also use the no-any rule in our linting. IMHO, use of any would defeat the purpose of TypeScript.

You might be able to suppress the noImplicitAny errors using the definite assignment assertion operator in your type definition?

The reason we need UMD instead of ESM is because we are targeting ES5. Using ESM would require transpiling everything in the browser, which slows down the app. If we used Webpack it wouldn't be a problem but....

We can't use Webpack in our app. We need to allow customers to just put javascript on the file system and have that code added to the app. So this app needs to discover and then dynamically load javascript modules at runtime. Using a bundler like Webpack won't work. (Or at least, I don't know how to achieve that with Webpack. The customer should not need to know about or use Webpack at all.)

I would be happy to contribute my little project to PatternFly. Just need to talk to someone about how we want to do it.

[ssilvert]

@priley86 Last but not least, where are we now? Have you been able to get rid of some or all of the errors when transpiling my project?

[ssilvert]

@priley86 What is the status of this? Did my last posts make sense to you?

[rachael-phillips]

Hi @ssilvert @patrick is on pto currently. cc @dana

[priley86]

i've made some more progress on this lately. Have converted the base Reactabular source to TSX. It compiles and UI runs fine, but tests currently break. Will continue converting the remaining source and existing tests to TypeScript...

c4e2d29