refactor: flow -> ts #121

agilgur5 · 2022-03-26T22:32:03Z

Summary

Converts JS/Flow files to TS with flowts and many manual interventions.
Adds support for TS in linting, formatting, building, and CI.

TODO

Merge deps: upgrade to Prettier v2 #120 first
Fix remaining type errors
Remove $FlowIgnore or replace with @ts-expect-error where still necessary
Remove Flow entirely~?~
- ~~I think this should be fine but not 100% sure that nothing uses it anymore~~ -- could not find other references to Flow (e.g. with // @flow comments), so removed it
- Remove from package.json, yarn.lock, babel.config.json, .flowconfig, ci.yml, package.json:scripts, CONTRIBUTING.md
~~Convert tests to TS?~~ No, see below
- flowts seemed to leave all of these alone. Fixtures should remain as JS, but test files themselves could be converted. Babel core seemed to still have tests in JS as well unless I missed something
  - Yep, there is a perf rationale for Babel tests to be in JS
~~add .git-blame-ignore-revs on top of the flowts commit?~~ No, this won't work anyway
- namely for the types.js big change due to , -> ; change

Details

add tsconfig.json and typescript dep
support linting and formatting TS files
- add @typescript-eslint for linting with type information
- add TS extensions to ESLint config and Prettier config
- fix no-unused-vars with custom rule similar to Babel core
support building TS files
- add @babel/preset-typescript to parse and build TS files similar to Babel core
- modify Gulpfile + Rollup config to handle TS similar to Babel core
- modify shipped-proposals script to output as TS
auto convert Flow -> TS via flowts
- some manual changes/adjustments:
  - manually reversed some incorrect formatting changes (lines with // eslint-ignore or // prettier-ignore)
  - manually reversed various unnecessary spacing / new line changes (as requested by Nicolò)
  - manually renamed some auto-named vars
- major changes seem to be:
  - removing // @flow at the top of files
  - * -> any
  - Object -> any
  - ?type -> type | undefined | null
  - using semicolons instead of commas for object property types
- 453 type errors post-conversion
manually remediate several type errors post-conversion
- went from 453 errors at the beginning to 40 errors!
- some missing types:
  - missing generics in a few places, particularly with Sets (e.g. Set<string>)
  - missing optional parameters in a few places (e.g. subfolder?)
  - missing default values in some initializers (e.g. { useBabelRuntime = "" })
- some fixes:
  - add | typeof presetEnvSilentDebugHeader to fix debug checks
  - add "object" in meta check to usageGlobal funcs to narrow type before assuming meta.object exists
  - one NodePath type import was incorrect / different from the rest
  - one hasOwnProperty was called as a global func instead of on Object or Object.prototype
- remove babel.default || babel imports
  - tsconfig esModuleInterop should handle this
remove or replace $FlowIgnore with @ts-expect-error
- in most places, there was no type error, so just removed the // $FlowIgnore comment
- in 2 places, there was a type error, so replace with a // @ts-expect-error comment instead
  - this could be removed with some type narrowing checks instead, but this is a more direct conversion (and requested in review comments below)
- 38 type errors remaining
refine non-trivial types to remediate remaining type errors
- add @types/babel__traverse as a devDep for more accurate typings
  - this was already a transitive dep, so adding it just updated it to a newer (and more accurate) version
- some complicated generics refined:
  - missing Options in regenerator's defineProvider call
  - many missing generics for various NodePath args
- had to switch up some isXXX() conditionals and surroundings to narrow the right type
  - unfortunately the type narrowing doesn't work on every property or to the entire depth of the tree 😕
- add a @ts-expect-error where proper type resolution was not possible with the current Babel types
- use casts where TS was not able to narrow the type all the way
support TS type-checking in CI
- add yarn tscheck command (same naming as Babel core)
- modify yarn validate to also run yarn tscheck
- modify ci.yml GitHub workflow to also run yarn tscheck
- add yarn tscheck to CONTRIBUTING.md
remove all Flow deps, scripts, config, CI, etc
- remove Flow dep and .flowconfig
- remove Flow scripts from package.json and ci.yml
- remove Babel config and Babel plugin
- ESLint plugin is required by eslint-config-babel, so cannot be removed

Review Notes

Can see each individual commit for more details and a more detailed commit description
I found Convert @babel/template from Flow to TS babel#12317 -- let me know if you'd like me to split more of the "prep" parts as separate PRs (all the env commits). I wasn't sure since you had said "single PR" in feat: better error message when no options are provided #118 (comment)
Still some TODOs left as per above

References

See thread at feat: better error message when no options are provided #118 (review) that kicked off the conversion. It references multiple things from Babel core's Flow -> TS migration:
deps: upgrade to Prettier v2 #120 and Update every @babel/* dependency #122 (ish) were split off from this

agilgur5

left some in-line comments for clarity/reference as well as some questions

.eslintrc.json

package.json

packages/babel-helper-define-polyfill-provider/src/utils.ts

packages/babel-helper-define-polyfill-provider/src/imports-cache.ts

packages/babel-plugin-polyfill-es-shims/src/index.ts

packages/babel-plugin-polyfill-corejs2/src/built-in-definitions.ts

packages/babel-helper-define-polyfill-provider/src/index.ts

packages/babel-helper-define-polyfill-provider/src/types.ts

packages/babel-plugin-polyfill-corejs3/src/built-in-definitions.ts

packages/babel-helper-define-polyfill-provider/src/types.ts

agilgur5

Investigated some of the type errors some more!

Left a few in-line comments on the remaining type errors.

Other than those, the rest are of the form, e.g. error TS2339: Property 'value' does not exist on type 'Node'. Property 'value' does not exist on type 'AnyTypeAnnotation'. -- basically the type has not been narrowed enough.

This could be solved with some narrowing checks, typecasts, // @ts-ignore, etc. Or may be due to incomplete type info somewhere?

packages/babel-helper-define-polyfill-provider/src/types.ts

packages/babel-helper-define-polyfill-provider/src/index.ts

packages/babel-plugin-polyfill-corejs2/src/index.ts

packages/babel-plugin-polyfill-corejs3/src/index.ts

agilgur5 · 2022-03-27T05:22:36Z

I'm also not entirely sure if the snapshot changes are correct or not... I actually hadn't seen them locally until recently.

Investigated that as I thought they might have came from my manual type remediations, but actually they seem to begin with the flowts commit 🤔
I probably missed them initially because you need to run yarn build in this repo before running yarn test, otherwise you end up testing stale code, which is probably what I had done locally while developing.

packages/babel-helper-define-polyfill-provider/src/imports-cache.ts

packages/babel-plugin-polyfill-corejs2/src/index.ts

nicolo-ribaudo · 2022-03-27T10:19:50Z

The failing tests are because of the Babel update; just rebase on main and the failures should go away.

- deps: add typescript - future commits will use this more - add a tsconfig.json for TS support - config as requested by Nicolò - plus some fixes to properly put `paths` in the right place

deps: add @typescript-eslint packages and config - to support linting and formatting TS files with type information - add .ts extension to eslint config and commands - add @typescript-eslint packages to config - fix `no-unused-vars` errors with custom rule similar to Babel core - correspondingly change `// eslint-disable-line` comments to use `@typescript-eslint/no-unused-vars` instead of `no-unused vars` - add .ts extension to .prettierrc - to support formatting TS files - and so that `flowts` will use this config during its formatting step as the files will have now been renamed with .ts extensions

deps: add @babel/preset-typescript to parse and build TS files - to continue working with rollup-plugin-babel - Babel core does this as well - and add it to babel.config.json - modify Gulpfile + Rollup config to handle TS - add TS extensions to Gulpfile globs - use index.ts as entrypoint now - this will be used in the next commit post-conversion - modify shipped-proposals script to output as TS - basically just no `// @flow` comment on top - and use `.ts` extension - also update comment to use the script's current extension, `.mjs`

test/esm/package.json

agilgur5

The failing tests are because of the Babel update

Ah right, I updated that later while developing and not during the initial conversions, so that explains it! Fixed now after rebase (it was a pretty complicated rebase though with all the formatting and package.json/yarn.lock changes).

Fixed several comments, various flowts formatting, and ~8 type errors, but unfortunately still 37 type errors left (and +10 if createDescIterator changes are made), almost entirely stemming from NodePath errors.
Per in-line comment, adding @types/babel__traverse only fixed 3 type errors 😕

packages/babel-plugin-polyfill-corejs3/src/index.ts

packages/babel-plugin-polyfill-es-shims/src/index.ts

packages/babel-helper-define-polyfill-provider/src/index.ts

packages/babel-plugin-polyfill-corejs2/src/index.ts

packages/babel-helper-define-polyfill-provider/src/index.ts

packages/babel-helper-define-polyfill-provider/src/utils.ts

nicolo-ribaudo

A few pointers to fix some of the TS errors:

packages/babel-helper-define-polyfill-provider/src/imports-cache.ts
- The various programPath parameters should be NodePath<t.Program>. Probably also a few weakmaps need to be updated
- The code in _injectImport is horribly written, probably we should split the usage of the lastImport variable in two variables (lastImport and newNodes)
packages/babel-helper-define-polyfill-provider/src/utils.ts
- The path param of resolveKey should be a NodePath<t.Expression>
- Using t.isXXX(node) instead of path.isXXX() in resolveKey will probably help with type refinements
- You might need a few as NodePath<t.Expression> after some .get("...") calls
- getImportSource's parameter should be a NodePath<t.ImportDeclaration>
- I don't think ._blockHoist is in the type definitions; just put a @ts-expect-error in front of it
packages/babel-helper-define-polyfill-provider/src/visitors/usage.ts
- All the visitor functions need a better NodePath annotation, for example MemberExpression(path: NodePath) should be MemberExpression(path: NodePath<t.MemberExpression>). The node for ReferencedIdentifier is just t.Identifier

agilgur5

~15ish type errors left 😅

Getting there. Think I might be able to get another half of them, but it's getting trickier with each refinement (and the remaining ones were already pretty tricky).

The tips helped a lot! Also the new type info made it easier for me to understand the AST parsing going on as well 🙂

packages/babel-helper-define-polyfill-provider/src/utils.ts

packages/babel-plugin-polyfill-corejs3/src/index.ts

- ran `flowts` - then manually reversed a few incorrect formatting - some lines that had `// eslint-ignore` or `// prettier-ignore` - and manually reversed some unnecessary spacing / new line changes - as requested by Nicolò - manually renamed some auto-named vars in parameter types - major changes seem to be: - removing `// @flow` at the top of files - `*` -> `any` - `Object` -> `any` - `?type` -> `type | undefined | null` - using semicolons instead of commas for object property types - this is just TS syntax vs. Flow syntax - this is the majority of reformatting and caused a types file to appear as deletion+addition instead of rename as the majority of lines were reformatted due to this change - 453 type errors post-conversion that need to be manually fixed

went from 453 type errors at the beginning to only 40 type errors now! - some missing types: - missing generics in a few places, particulary with Sets - e.g. `Set<string>` - missing optional parameters in a few places - e.g. `subfolder?` - missing default values in some initializers - e.g. `{ useBabelRuntime = "" }` - some small fixes: - add `| typeof presetEnvSilentDebugHeader` to fix debug checks - add `"object" in meta` check to `usageGlobal` funcs to narrow type before assuming `meta.object` exists - one NodePath type import was incorrect / different from the rest - one `hasOwnProperty` was called as a global func instead of on `Object` or `Object.prototype` - remove `babel.default || babel` imports - tsconfig `esModuleInterop` should handle this

- in most places, there was no type error, so just removed the `// $FlowIgnore` comment - in 2 places, there was a type error, so replace with a `// @ts-expect-error` comment instead - this could be removed with some type narrowing checks instead, but this is a more direct conversion - and requested by Nicolò - 38 type errors remaining

agilgur5

Ok, got to 9 type errors now (+7 if NodePath is added to createDescIterator in es-shims).
I've also added more types in various places where I inferred what it should be based on the new/existing patterns of typings (went from like ~200 type errors if you turn on strict to ~130 type errors)

Don't think I can get any farther without more "hints" as I don't actually know what the code should be. Like not sure if I should cast some things or if there's actually a way of narrowing properly (or if the type error is accurate).

Was guessing a bit (not great practice -- shows how much I'm stretching my knowledge of this codebase/Babel) and did some trial-and-error here and there that you might have seen in the intermediary pushes (e.g. adding too many generics?, changing isXXX() multiple times and breaking the tests a bit before fixing them, etc).
The types in imports-cache were particularly confusing and onerous to wrap my brain around, but got through all but one of them

packages/babel-helper-define-polyfill-provider/src/imports-cache.ts

packages/babel-helper-define-polyfill-provider/src/utils.ts

packages/babel-helper-define-polyfill-provider/src/types.ts

nicolo-ribaudo

Some more hint I could find looking at the ci TS errors.

In the next few days I will clone this PR locally to debug the remaining failures!

packages/babel-helper-define-polyfill-provider/src/utils.ts

packages/babel-helper-define-polyfill-provider/src/visitors/usage.ts

packages/babel-helper-define-polyfill-provider/src/utils.ts

packages/babel-plugin-polyfill-corejs2/src/index.ts

- deps: add `@types/babel__traverse` for more accurate typings - Babel 8 will produce its own typings - this was already a transitive dep, so adding it just updated it to a newer (and more accurate) version - some complicated generics refined: - missing `Options` in regenerator's `defineProvider` call - many missing generics for various `NodePath` args - thanks to Nicolò for help with some of these, I didn't know where to even start with these - had to switch up some `isXXX()` conditionals and surroundings to narrow the right type - unfortunately the type narrowing doesn't work on every property :/ - add a `@ts-expect-error` where proper type resolution was not possible with the current Babel types - use casts where TS was not able to narrow the type all the way - add some typings here and there as I went

- add `yarn tscheck` command - same naming as Babel core - modify `yarn validate` to also run `yarn tscheck` - modify ci.yml GitHub workflow to also run `yarn tscheck` - add `yarn tscheck` to CONTRIBUTING.md

- remove Flow dep and .flowconfig - remove Flow scripts from package.json and ci.yml - remove Babel config and Babel plugin - ESLint plugin is required by eslint-config-babel, so cannot be removed

agilgur5

We now have just 1 more type error remaining!

@nicolo-ribaudo could you go through the leftover unresolved discussions and either comment or react or resolve them? If I received a thumbs-up on some of my comments, I resolved them myself, but some got no reaction so I wasn't sure if you had seen them or not.
I made a list below to make it easier for you to check things off.

Leftover Unresolved Discussions

Remaining TODOs for me

~~Convert tests to TS? Per my PR description, flowts left them all alone, but I think they might be in JS in Babel core too~~
- There is a perf rationale for this
~~Split things into different PRs? (per PR description)~~
- Not needed
Flow's Maybe type refinement
- Completed by Nicolò in 80605cc

agilgur5 · 2022-04-01T19:03:33Z

packages/babel-plugin-polyfill-es-shims/src/index.ts

+      meta: MetaDescriptor,
+      resolved: any,
+      utils: Utils,
+      path: any,


Per prev discussion, I left this as any for now as changing it to just NodePath (with no generic or further specification) adds ~7 type errors

packages/babel-plugin-polyfill-corejs3/src/index.ts

nicolo-ribaudo · 2022-04-03T21:17:38Z

Oh thanks for that comment with all the things I didn't reply to 😅

.git-blame-ignore-revs

I don't think this will work anyway, because the file has also been renamed and git thinks that it's a new file, so git doesn't now how to track the original contents.

(I'm still going through the other checkboxes)

nicolo-ribaudo · 2022-04-03T22:38:38Z

Convert tests to TS? Per my PR description, flowts left them all alone, but I think they might in JS in Babel core too

I'd prefer to leave them as JS, so that in the future we can run them without on-the-fly transpilation and use github.com/nicolo-ribaudo/jest-light-runner (which I just extracted from the Babel repo) because it has better support for ESM compared to Jest.

Split things into different PRs? (per PR description)

Nah, this is still small enough that it's not needed.

This should be ready now, if I didn't miss something feel free to mark it as "ready" so that I can then merge it 🙂

agilgur5

Just one comment, otherwise I think it's good to go!

Thanks for sticking through this process with me @nicolo-ribaudo ! This was actually my first time touching any actual parser code in any Babel plugin (as opposed to everything else). Hopefully this experience makes it easier for me to write parser plugins myself in the future 🙂

Always a good experience interacting with you Nicolò ❤️

packages/babel-helper-define-polyfill-provider/src/normalize-options.ts

scripts/babel-plugin-compat-core-import.js

agilgur5 · 2022-04-07T17:24:05Z

Oh thanks for that comment with all the things I didn't reply to 😅

Glad to hear that the list I made was useful! 🙂

I don't think this will work anyway, because the file has also been renamed and git thinks that it's a new file, so git doesn't now how to track the original contents.

Ah, right I forgot it got counted an addition/deletion so it wouldn't be tracked as a same file change as such 😕 Might be caught by a different diff algorithm but don't think GitHub supports those yet. Good catch!

I'd prefer to leave them as JS, so that in the future we can run them without on-the-fly transpilation and use github.com/nicolo-ribaudo/jest-light-runner (which I just extracted from the Babel repo) because it has better support for ESM compared to Jest.

Thanks for the context! I figured there must be a reason for that, perf differential makes sense.
That being said, I've sometimes found having types in my tests to be useful for finding some errors, and type tests are a thing too. But probably more useful for Babel 8+

This was referenced Mar 26, 2022

deps: upgrade to Prettier v2 #120

Merged

feat: better error message when no options are provided #118

Merged

agilgur5 force-pushed the refactor-ts branch from 6a649a9 to d1bc5d3 Compare March 27, 2022 01:51

agilgur5 commented Mar 27, 2022

View reviewed changes

agilgur5 force-pushed the refactor-ts branch 2 times, most recently from 3b58b82 to e4c352d Compare March 27, 2022 05:09

agilgur5 commented Mar 27, 2022

View reviewed changes

nicolo-ribaudo reviewed Mar 27, 2022

View reviewed changes

packages/babel-helper-define-polyfill-provider/src/imports-cache.ts Outdated Show resolved Hide resolved

nicolo-ribaudo reviewed Mar 27, 2022

View reviewed changes

packages/babel-plugin-polyfill-corejs2/src/index.ts Show resolved Hide resolved

agilgur5 added 3 commits March 28, 2022 00:06

env: begin supporting TS files

ec1230d

- deps: add typescript - future commits will use this more - add a tsconfig.json for TS support - config as requested by Nicolò - plus some fixes to properly put `paths` in the right place

agilgur5 force-pushed the refactor-ts branch from e4c352d to 27c9abb Compare March 28, 2022 04:35

agilgur5 commented Mar 28, 2022

View reviewed changes

test/esm/package.json Show resolved Hide resolved

agilgur5 force-pushed the refactor-ts branch from 27c9abb to 6547eb2 Compare March 28, 2022 05:52

agilgur5 commented Mar 28, 2022

View reviewed changes

agilgur5 force-pushed the refactor-ts branch 2 times, most recently from 56945b6 to e204245 Compare March 28, 2022 06:34

nicolo-ribaudo reviewed Mar 28, 2022

View reviewed changes

agilgur5 force-pushed the refactor-ts branch 4 times, most recently from 1237ed7 to 0eff18a Compare March 28, 2022 21:53

agilgur5 commented Mar 28, 2022

View reviewed changes

packages/babel-helper-define-polyfill-provider/src/utils.ts Outdated Show resolved Hide resolved

packages/babel-helper-define-polyfill-provider/src/utils.ts Show resolved Hide resolved

packages/babel-plugin-polyfill-corejs3/src/index.ts Outdated Show resolved Hide resolved

agilgur5 force-pushed the refactor-ts branch 2 times, most recently from 8760f2c to 6f0d6c7 Compare March 30, 2022 00:22

agilgur5 added 2 commits March 29, 2022 20:30

agilgur5 force-pushed the refactor-ts branch 2 times, most recently from 2e6f5c8 to 88babeb Compare March 30, 2022 01:41

agilgur5 commented Mar 30, 2022

View reviewed changes

packages/babel-helper-define-polyfill-provider/src/imports-cache.ts Show resolved Hide resolved

packages/babel-helper-define-polyfill-provider/src/utils.ts Show resolved Hide resolved

packages/babel-helper-define-polyfill-provider/src/types.ts Outdated Show resolved Hide resolved

nicolo-ribaudo reviewed Mar 30, 2022

View reviewed changes

agilgur5 force-pushed the refactor-ts branch from 88babeb to 4a35ae1 Compare April 1, 2022 18:53

agilgur5 added 3 commits April 1, 2022 15:04

ci: support TS type-checking in CI

2ed56d0

- add `yarn tscheck` command - same naming as Babel core - modify `yarn validate` to also run `yarn tscheck` - modify ci.yml GitHub workflow to also run `yarn tscheck` - add `yarn tscheck` to CONTRIBUTING.md

clean: remove all Flow deps, scripts, config, CI, etc

7dc712f

- remove Flow dep and .flowconfig - remove Flow scripts from package.json and ci.yml - remove Babel config and Babel plugin - ESLint plugin is required by eslint-config-babel, so cannot be removed

agilgur5 force-pushed the refactor-ts branch from 4a35ae1 to 7dc712f Compare April 1, 2022 19:04

agilgur5 commented Apr 1, 2022

View reviewed changes

nicolo-ribaudo added 4 commits April 3, 2022 23:32

Minor updates

08d7329

Revert targets type

ff3432b

Fix ESM building

68f2796

Simplify | null | undefined

80605cc

agilgur5 commented Apr 7, 2022

View reviewed changes

packages/babel-helper-define-polyfill-provider/src/normalize-options.ts Show resolved Hide resolved

scripts/babel-plugin-compat-core-import.js Show resolved Hide resolved

agilgur5 marked this pull request as ready for review April 7, 2022 17:24

nicolo-ribaudo merged commit 4f49c9c into babel:main Apr 7, 2022

agilgur5 mentioned this pull request Nov 9, 2024

Polyfills are not generated from .browserslistrc queries jaredpalmer/tsdx#1132

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: flow -> ts #121

refactor: flow -> ts #121

agilgur5 commented Mar 26, 2022 •

edited

Loading

agilgur5 left a comment

agilgur5 left a comment

agilgur5 commented Mar 27, 2022

nicolo-ribaudo commented Mar 27, 2022

agilgur5 left a comment •

edited

Loading

nicolo-ribaudo left a comment

agilgur5 left a comment •

edited

Loading

agilgur5 left a comment •

edited

Loading

nicolo-ribaudo left a comment

agilgur5 left a comment •

edited

Loading

agilgur5 Apr 1, 2022

nicolo-ribaudo commented Apr 3, 2022

nicolo-ribaudo commented Apr 3, 2022

agilgur5 left a comment

agilgur5 commented Apr 7, 2022

refactor: flow -> ts #121

refactor: flow -> ts #121

Conversation

agilgur5 commented Mar 26, 2022 • edited Loading

Summary

TODO

Details

Review Notes

References

agilgur5 left a comment

Choose a reason for hiding this comment

agilgur5 left a comment

Choose a reason for hiding this comment

agilgur5 commented Mar 27, 2022

nicolo-ribaudo commented Mar 27, 2022

agilgur5 left a comment • edited Loading

Choose a reason for hiding this comment

nicolo-ribaudo left a comment

Choose a reason for hiding this comment

agilgur5 left a comment • edited Loading

Choose a reason for hiding this comment

agilgur5 left a comment • edited Loading

Choose a reason for hiding this comment

nicolo-ribaudo left a comment

Choose a reason for hiding this comment

agilgur5 left a comment • edited Loading

Choose a reason for hiding this comment

Leftover Unresolved Discussions

Remaining TODOs for me

agilgur5 Apr 1, 2022

Choose a reason for hiding this comment

nicolo-ribaudo commented Apr 3, 2022

nicolo-ribaudo commented Apr 3, 2022

agilgur5 left a comment

Choose a reason for hiding this comment

agilgur5 commented Apr 7, 2022

agilgur5 commented Mar 26, 2022 •

edited

Loading

agilgur5 left a comment •

edited

Loading

agilgur5 left a comment •

edited

Loading

agilgur5 left a comment •

edited

Loading

agilgur5 left a comment •

edited

Loading