-
Notifications
You must be signed in to change notification settings - Fork 376
Description
What
When running yarn start contributors do not see tsc errors until submitting a PR or manually running yarn build, which is confusing. All Typescript files are compiled twice (once to JS and once to .d.ts) so compilation takes twice as long as necessary.
Our current incremental build script is slow since it has to regenerate the source of react-icons each build. Also its terminal output confusing: when running yarn start we miss a crucial piece of information: that the dev server runs at http://localhost:8000.
History
We took on technical debt when we started adopting tsc in our repo to emit types from Typescript (.ts or .tsx) files in #884. Each Typescript file in our repo is compiled twice, first with babel, and then a second time with tsc to create .d.ts files. Now that the majority of our code is Typescript, compilation takes twice as long as necessary.
Since we have a monorepo, we also require support for incremental builds. This way touching react-core doesn't require building all packages. #1817 originally implemented our custom incremental build script.
Why
The benefits of this change would be:
- To save patternfly-react developers time on initial
yarn start/yarn build- A full build with
tsc --buildtakes just 30s on cold start with 2 threads
- A full build with
- To move building all packages at once to a standard system of
tsc --buildtsc --buildis better optimized to only rebuild when necessary and allows for cross-package typechecking- A rebuild with
tsc --buildtakes just 6s when touching a single file inreact-core
- Emit cleaner code in our
distthat makes Webpack's life easier and performs better in the browserbabelcurrently is emitting code older than ES2015 bloated with helpers at the top
- Type-checking on save
- Ctrl+click on cross-project types with
.d.tssourcemap support - Open up possibilities of writing tests and examples in TypeScript, improves support for
ts-jestto type check our tests in the future
How
I propose we move off of lerna run build, which runs babel 3 times in each of our packages to tsc --build to run a tsc incremental build once across all of our packages. The only thing we lose in this conversion is support for babel-plugin-typescript-to-proptypes. Per discussion on Slack in #patternfly-react, now that Typescript conversion is complete, our types are a good substitute for prop-types for now. Adopting something like ts-proptypes-transformer may be good future work.
Epic
| Issue | PRs | Completed | Description |
|---|---|---|---|
| #4042 | #4048 | ✔️ | Improve eslint speed on .tsx? files to something more reasonable than the over an hour it currently takes |
| #4043 | #4073 | ✔️ | Convert react-inline-edit-extension to Typescript and drop prop-types from it |
| #4044 | #4058 (tokens) #4066 (styles) #4069 (icons) | ✔️ | Make JS/TS generated in react-tokens, react-styles, and react-icons consistent with tsc (closes #4023 as well) |
| #4045 | #4050 | ✔️ | Correctly type all function components |
| (If needed for #4046) Upgrade Jest | |||
| #4049 | #4051 | ✔️ | Make imports compatible with tsc |
| #4046 | #4076 | ✔️ | Move to tsc --build (closes #3763 as well) |
| #4047 | Add list of supported ES2015 browsers to README.md |
Depends on #3977