Skip to content

Conversation

@redallen
Copy link
Contributor

@redallen redallen commented Apr 1, 2020

What: Closes #3972

  • Use tsc instead of babel to build react-* packages.
    • Add tslib dependency and use it via importHelpers: true flag to avoid hundreds of spread/import helpers at the top of most files.
    • Remove propTypes as a peerDependency since the babel plugin typescript-to-proptypes can no longer be used.
    • Add newly generated dist/**/*.map and dist/*.tsbuildinfo files to each project's .npmignore.
  • Remove custom incremental build script in favor of tsc's project references which are well suited for monorepos. Cold build times for ESM and CJS are ~60s. Hot build times when touching react-core are ~6s.
  • Drop per-package UMD output in favor of feat(rollup): create unified react-core dist #3971 since there's lots of overhead and it's inadvisable to use UMD imports that make 50+ web requests. Bundles should be used instead.
  • Change generate script to generate entire dist folder for react-tokens, react-icons and react-styles without using babel. Refactor code generation to match tsc's generated code.
    • Rename old yarn generate command (which ran plop) to yarn template or yarn plop.
      • Update plop's templates.
  • Convert last JS project react-inline-edit-extension to TS so it can compile without enabling tsc's allowJS flag.

Additional issues:

  • About a dozen function components (like TooltipArrow) were not defined as function components. I added types for these.
  • react-inline-edit-extension had > 200 type errors when setting allowJS: true. I refactored it to TS and remove propTypes from it to match the rest of our projects.
  • The linter in CI took >10m because I added/changed too many *.tsx files. I've removed the eslint typescript parser's "project" option for the time being which brings down the cold run time to ~50s and hot run time to 1.5s.

Further work:

  • Replace import * as React from 'react'; with import React from 'react';
  • Use ts-jest instead of babel-jest to type check test files while compiling them. Combined with @jschuler 's auto generated snapshot tests this eliminates the need for react-integration to type check.
  • Convert JS examples to TS and compile to JS using tsc.
  • Add list of supported ES2015 browsers to README.md

Breaking changes

@patternfly/react-virtualized-extension

  • Many types have been added and changed while converting this project to TS.

ALL PACKAGES

  • We no longer support UMD builds for individual packages. Use our new react-core.umd.js bundle instead.
  • We no longer define propTypes for our components. Consider using our Typescript types under dist/js instead.

@patternfly-build
Copy link
Collaborator

patternfly-build commented Apr 2, 2020

Copy link
Member

@ddonahue007 ddonahue007 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@seanforyou23 seanforyou23 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A big diff.. can we split out the work of upgrading inline-edit-extension to TS? Also, might be good to squash the commits.

@redallen
Copy link
Contributor Author

redallen commented Apr 6, 2020

@seanforyou23 It'd take a couple hours to split out the inline-edit-extension work and I'd have to get it to work with babel rather than tsc... but I'm happy to spend time doing that if getting it working with babel is valuable and then rebasing this PR on that.

There's a couple of merge commits in the history, so I can't easily squash them. Github will do that hard work upon merging, sorry for having to scroll to see the comments at the bottom here.

@mturley
Copy link
Collaborator

mturley commented Apr 6, 2020

Do we know if switching from babel to tsc will have any implications on the behavior of the compiled code? I have heard about subtle caveats, but I think they are mostly things tsc supports that babel doesn't. I can't find it now but someone told me about an issue where async/await code wasn't compiled exactly the same between them.

@redallen
Copy link
Contributor Author

redallen commented Apr 6, 2020

@mturley Yes our compiled code is different. This PR targets ES2015 and use Typescript helpers and transforms to compile our code rather than Babel's. This allows for the use of arrow functions, although I'm not sure about async/await (but we don't have any usages of async/await in our exported codebase). Overall, I think our generated code quality has increased. Here's a diff of esm/components/Brand/Brand.js, and another of js/components/Brand/Brand.js (Babel is on the left, and tsc + tslib is on the right). I encourage testing out all the components in https://patternfly-react-pr-4010.surge.sh to make sure there aren't any obvious regressions!

Also now that we clearly target ES2015 we can list browsers + versions that we support. I'll add that to the future work section of the OP.

@mturley
Copy link
Collaborator

mturley commented Apr 6, 2020

Nice! Yeah, definitely an improvement.

@seanforyou23
Copy link
Contributor

Drop per-package UMD output in favor of #3971

This is the first time I've heard of this, have we checked with relevant teams who depend on this feature?

It'd take a couple hours to split out the inline-edit-extension work and I'd have to get it to work with babel rather than tsc... but I'm happy to spend time doing that if getting it working with babel is valuable

I think you miss the point, it's not about getting the extension to work with babel but rather not sending a PR that's so large in volume that it becomes difficult to read and follow.

@mturley
Copy link
Collaborator

mturley commented Apr 7, 2020

@redallen do you mean you'd have to get the TS version of inline-edit-extension to work with babel in order to merge that before this PR? Could we instead leave its JS as-is for now but compiled by tsc (merge this PR with the allowJS flag set), and then have another PR to convert inline-edit-extension to TS and unset allowJS?

@mturley
Copy link
Collaborator

mturley commented Apr 7, 2020

Oh, I'm seeing this now:

react-inline-edit-extension had > 200 type errors when setting allowJS: true

Maybe we can just have TS ignore that package entirely for this PR but then how would it be bundled without babel... I get it now, ignore me.

@mturley
Copy link
Collaborator

mturley commented Apr 7, 2020

@redallen when you were using allowJs, did you also have checkJs on? According to microsoft/TypeScript#19573 (comment) we should be able to allow JS without emitting type errors within it.

Even if we can't do it that way, maybe we can use // @ts-nocheck in each of those files for this PR. https://github.com/microsoft/TypeScript/wiki/Type-Checking-JavaScript-Files

@redallen
Copy link
Contributor Author

redallen commented Apr 8, 2020

I'm turn #3972 into an epic and break up this PR into smaller pieces that are easier to understand and review.

@redallen redallen closed this Apr 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove babel in favor of tsc

6 participants