[ADS-6363][ASC-22] feat: add type definitions #1350

renrizzolo · 2022-02-06T23:48:44Z

Description

Generate type definition files for all components
Add scripts to facilitate automatic type generation
- created a babel plugin to 'normalize' the PropTypes, then use react-to-typescript-definitions to convert PropTypes to types. Types are saved to d.ts files alongside each component.
- some manual overrides are added so 3rd party lib types are applied - scripts/generate-types/typesPostFixes.js

A few surface changes were done for all of this to operate smoothly

make all component names consistent - the exports need to match the type defs, and they are generated from the component name
remove static class properties
simplify some prop type definitions that were too complicated to parse

Benefits

Type definitions unlock a huge boost for developer experience, even for plain JS projects. That's the driving motivator behind this PR.

With a typescript enabled code editor like VSCode, all component props and their definitions and descriptions are available and will autocomplete. Not only is this a huge productivity boost, it also reduces the chance for misspelled props or prop values.

The gifs below show some examples of: prop autocompletion/suggestion, prop descriptions via comments, type autocompletion/suggestion, type checking, member expression types. (// @ts-check is not needed, I'm just using it here to show when types would be incorrect.)

Usage

Generate types for all components
npm run generate-types

More...

Generate types for a single component
npm run generate-types -- --only=Button

Generate types for a single component (this will run for all jsx files in the folder matching Button)
npm run generate-types -- --only=Button

Generate types for a specific file
npm run generate-types -- --only=Button/index.jsx

globs are accepted:
npm run generate-types -- --only="RichTe*

Copy generated types to es/*
npm run generate-types -- --copy

Generate types with debugging output
npm run generate-types -- --debug

Check validity of generated types
npm run type-check

Proposed workflow for updating/adding types

Maintaining consistency of the type definitions is important, and it's up to us as contributors to do so.

Note that I haven't added generate-types to the dist script. This is to avoid any unintentional breaking changes slipping through. The onus is on the dev to generate and commit type definitions when making changes to or adding new components/props.

In most cases it will be as simple as running npm run generate-types, then verifying and committing the changes.

Of course, the Prop Types themselves are responsible for the type definitions, so please make sure Prop Types are as correct as possible (e.g, if you know the prop accepts certain strings only, the definition should be PropTypes.oneOf(['string1', 'string2']) as opposed to PropTypes.string

Prop descriptions

Comments (a la JSDoc) above a prop type will carry across to the type def, so feel free to describe the prop there to help other users!

Advanced usage

Advanced usage, e.g when wrapping types of a 3rd party library into a component, should be manually adjusted in scripts/generate-types/typesPostFixes.js.

Caveats

nested PropTypes.shape will not work
JS in prop type definitions won't be evaluated (see below)

This works:

const reusableTypes = ['primary', 'secondary', 'warning']
Component.PropTypes = {
  theme: PropTypes.shape(reusableTypes)
}

This doesn't work:

const reusableTypes = ['primary', 'secondary', 'warning']
const filteredTypes = reusableTypes.filter((type) => type !== 'warning')
Component.PropTypes = {
 theme: PropTypes.oneOf(filteredTypes),
}

Other options to explore/discuss

now we have a baseline of types, forget the auto-generation, and manually curate them
as above but create a definitely typed repo @types/adslot-ui to separate them further
convert the lib to typescript entirely

I feel that 1 would be potentially better due to being able to define the types as precisely as we need, without having to rely on modifications or be limited by the PropTypes lib – but would require more work and TS knowledge from contributors.

Does this PR introduce a breaking change?

Yes
No

Any typescript projects using adslot-ui may need to update types accordingly. E.g if custom types were added they should be replaced with the types from the library.

Manual testing step?

Screenshots (if appropriate):

codecov · 2022-02-07T00:00:49Z

Codecov Report

Merging #1350 (be74499) into master (e8a4fa0) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master     #1350   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           94        92    -2     
  Lines         1429      1429           
  Branches       362       362           
=========================================
  Hits          1429      1429

Impacted Files	Coverage Δ
src/components/Breadcrumb/index.jsx	`100.00% <ø> (ø)`
src/components/Checkbox/index.jsx	`100.00% <ø> (ø)`
src/components/TextEllipsis/index.jsx	`100.00% <ø> (ø)`
src/components/AlertInput/index.jsx	`100.00% <100.00%> (ø)`
src/components/ButtonGroup/index.jsx	`100.00% <100.00%> (ø)`
src/components/Carousel/index.jsx	`100.00% <100.00%> (ø)`
src/components/ConfirmModal/index.jsx	`100.00% <100.00%> (ø)`
src/components/FilePicker/index.jsx	`100.00% <100.00%> (ø)`
src/components/FormGroup/index.jsx	`100.00% <100.00%> (ø)`
...onents/HoverDropdownMenu/PopoverLinkItem/index.jsx	`100.00% <100.00%> (ø)`
... and 17 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e8a4fa0...be74499. Read the comment docs.

scripts/generate-types/copyTypes.js

xiaofan2406

This is great work. 👍

i will find some time to locally test this.

renrizzolo · 2022-02-14T02:10:57Z

scripts/build.js

    // Merge with the public folder
-    if (process.env.NODE_ENV === 'production') {
-      process.env.DEMO_ASSETS ? copyDemoAssets() : copyPublicFolder();


I don't think copyPublicFolder() is ever used (DEMO_ASSETS and production are always used together).
Can anyone confirm?

shn2016 · 2022-03-03T23:59:15Z

src/components/Alert/index.d.ts

+export interface AlertProps {
+  /**
+   * ['success', 'info', 'warning', 'danger']
+   */


this comment is a bit duplicated though, so is other similar one

The redundant prop comments are for to the docs site's prop descriptions. The type generation just pulls all the comments from the prop types.

When we move to storybook, the prop values will be extracted properly so we'll be able to remove these comments then.

src/components/Breadcrumb/index.d.ts

src/components/Checkbox/index.d.ts

shn2016 · 2022-03-04T00:25:56Z

Great work!

renrizzolo · 2022-04-04T03:12:35Z

scripts/generate-types/generateTypes.js

+        console.log(chalk.green.bold(`Generated type defs for ${component.componentName}`));
+
+        const output = typesPostFixes(component.componentName, result);
+        const prettifiedOutput = prettier.format(output, { parser: 'typescript', ...pkg.prettier });


added prettier settings from package.json so output is consistently formatted

renrizzolo force-pushed the type-defs--on-esm-branch branch from b3e0ab7 to 45ec224 Compare February 6, 2022 23:57

renrizzolo requested review from xiaofan2406 and knilink February 7, 2022 00:29

renrizzolo changed the title ~~[ASC-22] feat: add type definitions~~ [ADS-6363][ASC-22] feat: add type definitions Feb 7, 2022

knilink reviewed Feb 11, 2022

View reviewed changes

scripts/generate-types/copyTypes.js Outdated Show resolved Hide resolved

xiaofan2406 reviewed Feb 11, 2022

View reviewed changes

renrizzolo force-pushed the type-defs--on-esm-branch branch 3 times, most recently from cf74a51 to be80af7 Compare February 14, 2022 01:38

renrizzolo commented Feb 14, 2022

View reviewed changes

bchew requested review from xiaofan2406 and knilink March 3, 2022 23:10

vinteo previously approved these changes Mar 3, 2022

View reviewed changes

shn2016 reviewed Mar 3, 2022

View reviewed changes

shn2016 reviewed Mar 4, 2022

View reviewed changes

src/components/Breadcrumb/index.d.ts Outdated Show resolved Hide resolved

shn2016 reviewed Mar 4, 2022

View reviewed changes

src/components/Checkbox/index.d.ts Outdated Show resolved Hide resolved

shn2016 previously approved these changes Mar 4, 2022

View reviewed changes

xiaofan2406 previously approved these changes Mar 4, 2022

View reviewed changes

renrizzolo dismissed stale reviews from xiaofan2406, shn2016, and vinteo via 4565d36 March 4, 2022 01:42

renrizzolo force-pushed the type-defs--on-esm-branch branch from 444df32 to 4565d36 Compare March 4, 2022 01:42

renrizzolo force-pushed the type-defs--on-esm-branch branch 3 times, most recently from e1b0916 to b1bb5e3 Compare April 4, 2022 03:09

renrizzolo commented Apr 4, 2022

View reviewed changes

renrizzolo force-pushed the type-defs--on-esm-branch branch 2 times, most recently from b13c4db to afeb5a3 Compare April 5, 2022 00:04

renrizzolo added 2 commits April 5, 2022 10:14

feat: generate types

3d29642

chore: generate types 🎉

be74499

renrizzolo force-pushed the type-defs--on-esm-branch branch from afeb5a3 to be74499 Compare April 5, 2022 00:16

xiaofan2406 approved these changes Apr 5, 2022

View reviewed changes

knilink approved these changes Apr 5, 2022

View reviewed changes

xiaofan2406 merged commit ec1389a into master Apr 5, 2022

xiaofan2406 deleted the type-defs--on-esm-branch branch April 5, 2022 04:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ADS-6363][ASC-22] feat: add type definitions #1350

[ADS-6363][ASC-22] feat: add type definitions #1350

renrizzolo commented Feb 6, 2022

codecov bot commented Feb 7, 2022 •

edited

Loading

xiaofan2406 left a comment

renrizzolo Feb 14, 2022

shn2016 Mar 3, 2022

renrizzolo Mar 4, 2022

renrizzolo Mar 4, 2022

shn2016 commented Mar 4, 2022

renrizzolo Apr 4, 2022

[ADS-6363][ASC-22] feat: add type definitions #1350

[ADS-6363][ASC-22] feat: add type definitions #1350

Conversation

renrizzolo commented Feb 6, 2022

Description

Benefits

Usage

Proposed workflow for updating/adding types

Prop descriptions

Advanced usage

Caveats

Other options to explore/discuss

Does this PR introduce a breaking change?

Manual testing step?

Screenshots (if appropriate):

codecov bot commented Feb 7, 2022 • edited Loading

Codecov Report

xiaofan2406 left a comment

Choose a reason for hiding this comment

renrizzolo Feb 14, 2022

Choose a reason for hiding this comment

shn2016 Mar 3, 2022

Choose a reason for hiding this comment

renrizzolo Mar 4, 2022

Choose a reason for hiding this comment

renrizzolo Mar 4, 2022

Choose a reason for hiding this comment

shn2016 commented Mar 4, 2022

renrizzolo Apr 4, 2022

Choose a reason for hiding this comment

codecov bot commented Feb 7, 2022 •

edited

Loading