Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve traverse typings #14648

Merged
merged 16 commits into from Jun 21, 2022
Merged

Conversation

JLHwung
Copy link
Contributor

@JLHwung JLHwung commented Jun 8, 2022

Q                       A
License MIT

See #14601 for more info. This PR improves @babel/traverse typings and fixes other package typing errors due to more effective traverse typings.

@babel-bot
Copy link
Collaborator

babel-bot commented Jun 8, 2022

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/52316/

: // todo: refine to t.Node[]
// ? E extends t.Node
// ? E | E[]
// : never
Copy link
Contributor Author

Choose a reason for hiding this comment

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

unshiftContainer now enforces stricter typings. I tried refining further but TS fails to infer statements: Statement[] can be passed to NodePath<t.BlockStatement>.unshiftContainer("body", statements)

@@ -338,8 +373,9 @@ export function pushContainer(
const path = NodePath.get({
parentPath: this,
parent: this.node,
container: container,
container: container as unknown as t.Node | t.Node[],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have to cast it as unknown first because NodePath.get does not have template typings.

| t.CatchClause,
>(
this: NodePath<T>,
): asserts this is NodePath<T & { body: t.BlockStatement }>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ensureBlock now asserts the shape { body: t.BlockStatement }, however TS requires explicit type annotations for assertions so I added some type annotations in other plugins.

Copy link
Member

Choose a reason for hiding this comment

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

We could also change it to return undefined in Babel 8 maybe.

let inferer = inferers[node.type];
let inferer =
// @ts-expect-error inferers do not cover all AST types
inferers[node.type];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The inferers could be refined to Record<node["type"], Inferer> but then we have to convert the namespace exports in inferers to a plain object export.

@@ -204,7 +205,7 @@ function gatherNodeParts(node: t.Node, parts: any[]) {
break;

case "JSXOpeningElement":
parts.push(node.name);
gatherNodeParts(node.name, parts);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a bug fix: node.name is an AST node.

@@ -25,7 +25,7 @@
},
"dependencies": {
"@babel/helper-validator-identifier": "workspace:^",
"to-fast-properties": "condition:BABEL_8_BREAKING ? ^4.0.0 : ^2.0.0"
"to-fast-properties": "condition:BABEL_8_BREAKING ? ^3.0.0 : ^2.0.0"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

const VALID_CALLEES = ["String", "Number", "Math"] as const;
const INVALID_METHODS = ["random"] as const;

function isValidCalless(val: string): val is typeof VALID_CALLEES[number] {
Copy link
Member

Choose a reason for hiding this comment

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

isValidCalless
Is it a typo?

@@ -51,7 +51,7 @@ export default function splitExportDeclaration(
}

const updatedDeclaration = standaloneDeclaration
? declaration
? declaration.node
Copy link
Member

Choose a reason for hiding this comment

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

This looks like a bugfix, did it change any observable behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not a bug fix. path.replaceWith accepts either Node or NodePath, but the current signature

replaceWith<R extends t.Node>(
  this: NodePath,
  replacementPath: R | NodePath<R>,
): [NodePath<R>]

does not support NodePath<A> | B. So here I ensure updatedDeclaration is always a Node.

@@ -531,7 +531,7 @@ export default declare((api, opts: Options) => {
},

// taken from transform-destructuring/src/index.js#visitor
ForXStatement(path) {
ForXStatement(path: NodePath<t.ForXStatement>) {
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be automatically inferred given the visitor name? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, TS already inferred the path type, however, the TS assertions in ensureBlock requires that the path has an explicit type annotation. I think it is a TS limitation. See also #14648 (comment).

@JLHwung
Copy link
Contributor Author

JLHwung commented Jun 20, 2022

I have rebased noImplicitAny and this branch. Hopefully CI will be green again.

@JLHwung JLHwung force-pushed the noImplicitAny-traverse branch 2 times, most recently from 8160a4b to f3cf592 Compare June 20, 2022 15:36
if (!validateError(error)) {
// @ts-ignore Fixme: validateError can be a string | object
// see https://nodejs.org/api/assert.html#assertrejectsasyncfn-error-message
if (typeof validateError === "function" && !validateError(error)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TS caught some bugs in our polyfill. We can fix this in a separate PR or leave it as-is.

Copy link
Member

Choose a reason for hiding this comment

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

It works well enough for what we need 😛

)
) {
function getLastStatement(statement: t.Statement): t.Statement {
// @ts-ignore: If statement.body is empty or not a Node, isStatement will return false
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have reverted some changes suggested before since getLastStatement does accept a BreakStatement, it will return the statement itself as the "last statement".

@JLHwung
Copy link
Contributor Author

JLHwung commented Jun 20, 2022

On the failing Babel 8 test: we should probably run make tscheck after stripping Babel_8_BREAKING flags.

Comment on lines 78 to 88
if (!path.isFunctionDeclaration() && !path.isClassDeclaration()) return;
if (this.binding.kind !== "hoisted") return;

path.node.id = identifier(this.oldName);
path.node._blockHoist = 3;

path.replaceWith(
variableDeclaration("let", [
variableDeclarator(identifier(this.newName), toExpression(path.node)),
]),
);
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible that we'll need this code in the future? If yes, can we keep it commented out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I make it an identity function so TS will not complain unused params.

@JLHwung JLHwung merged commit 03880bf into babel:noImplicitAny Jun 21, 2022
@JLHwung JLHwung deleted the noImplicitAny-traverse branch June 21, 2022 15:43
JLHwung added a commit that referenced this pull request Jun 21, 2022
* export removeProperties options

* traverse

* fix other packages typings

* fix gatherNodeParts

* downgrade to-fast-properties to v3

* fix typo

* babel-core/normalize-file

* add ignore comment property used by flow plugin

* refine getLastStatement typings

* fix babel-standalone rebase typing error

* fix assert.rejects polyfill

* simplify _param typings

* supress Babel 7 AST errors

* loosen defineType typings

The AST typings are generated from defineType calls, so defineType should accept a string as type.

* restore legacy code

* Suppress data/core-js-compat importing errors
JLHwung added a commit that referenced this pull request Jun 21, 2022
* enable noImplicitAny

* codemod (#14602)

* Improve preset/plugin-typescript typings (#14603)

* preset-typescript

* transform-typescript

* Update packages/babel-plugin-transform-typescript/src/enum.ts

Co-authored-by: Nicolò Ribaudo <nicolo.ribaudo@gmail.com>

* prettier

Co-authored-by: Nicolò Ribaudo <nicolo.ribaudo@gmail.com>

* Improve transform-runtime typings (#14605)

* enable noImplicitAny

* codemod (#14602)

* Improve preset/plugin-typescript typings (#14603)

* preset-typescript

* transform-typescript

* Update packages/babel-plugin-transform-typescript/src/enum.ts

Co-authored-by: Nicolò Ribaudo <nicolo.ribaudo@gmail.com>

* prettier

Co-authored-by: Nicolò Ribaudo <nicolo.ribaudo@gmail.com>

* improve transform-runtime typings

Co-authored-by: Nicolò Ribaudo <nicolo.ribaudo@gmail.com>

* jsx (#14607)

* Improve module transform packages typings (#14608)

* improve babel-template typings (#14621)

* Improve @babel/helper-* typings (#14620)

* improve helper-function-name typings

* helper hoist variables

* helper-member-expression-to-functions

* helper-simple-access

* remap-async-to-generator

* helper-annotate-as-pure

* helper-builder-binary-assignment-operator-visitor

* helper-check-duplicate-nodes

* early return when export declaration is export all

* split-export-declaration

* helper-define-map

* define-map

* Update packages/babel-helper-builder-binary-assignment-operator-visitor/src/index.ts

Co-authored-by: Nicolò Ribaudo <nicolo.ribaudo@gmail.com>

* review comments

Co-authored-by: Nicolò Ribaudo <nicolo.ribaudo@gmail.com>

* improve helper-module-imports typings (#14623)

* refactor: simplify ImportInjector._applyDefaults

* helper-module-imports

* map globals to the one used in Babel 8

* Improve fixture-test-runner typings (#14625)

* plugin-test-runner

* fixture-test-runner

* Update packages/babel-helper-fixtures/src/index.ts

Co-authored-by: Nicolò Ribaudo <nicolo.ribaudo@gmail.com>

Co-authored-by: Nicolò Ribaudo <nicolo.ribaudo@gmail.com>

* Improve preset-env typings (#14606)

* refactor: move shipped-proposals to ts

* preset-env

* address review comments

* Improve pipeline-operator typings (#14629)

* refactor: simplify buildOptimizedSequenceExpression

* proposal-pipeline-operator

* address review comments

* improve standalone typings (#14630)

* babel-standalone

* address review comment

* Improve plugins typings (Part 1) (#14633)

* change AssumptionFunction return type

* helper-create-regexp-features-plugin

* create-class-features-plugin

* transform-for-of

* improve unicode-escapes typings

* transform-object-super

* transform-react-constant-elements

* proto-to-assign

* function-name

* flow-comments

TS cannot infer Flow visitor type because we have both Flow type and Flow virtual type.

* Improve code-frame/hightlight typings (#14643)

* code-frame

* map js-tokens to Babel 8

* highlight

* Improve `@babel/types` typings (#14645)

* bump to-fast-properties to v4

* types

* mark node comments as mutable

* Improve babel-core typings (#14622)

Co-authored-by: Nicolò Ribaudo <nicolo.ribaudo@gmail.com>

* improve transform-classes typings (#14624)

* Improve `@babel/generator` typings (#14644)

* generator

* refactor: merge {start,end}Terminatorless to printTerminatorless

* inline buildYieldAwait

* inline ExportDeclaration

* also export Pos

* let getPossibleRaw return string | void

* Apply suggestions from code review

Co-authored-by: Nicolò Ribaudo <nicolo.ribaudo@gmail.com>
Co-authored-by: liuxingbaoyu <30521560+liuxingbaoyu@users.noreply.github.com>

* address review comments

* do not export internal printer method

* simplify needsWhitespace

Co-authored-by: Nicolò Ribaudo <nicolo.ribaudo@gmail.com>
Co-authored-by: liuxingbaoyu <30521560+liuxingbaoyu@users.noreply.github.com>

* Improve plugins typings (Part 2) (#14639)

* exponentiation-operator

* duplicate-keys

* computed-properties

* replace-supers

* block-scoping

* block-scoped-functions

* syntax-typescript

* record-and-tuple

* private-property-in-object

* partial-application

* json-strings

* function-sent

* function-bind

* class-static-block

* async-generator-functions

* Update packages/babel-plugin-transform-block-scoping/src/index.ts

* address review comments

* helper-replace-supers

* Improve plugins typings (Part 3) (#14642)

* external-helpers

* bugfix

* transform-parameters

* object-rest-spread

* destructuring-private

* transform-destructuring

* proposal-decorators

* optional-chaining

* helper-wrap-function

* explode-assignable-expression

* helper-compilation-targets

* helper-plugin-utils

* helpers

* helper-validator-option

* fix: allow "+" and "-" in MappedType .readonly/.optional

* fixture-test-runner

* remove charcodes from dependencies

the charcodes transform will inline charCodes.* so we can remove it from dependencies

* Improve traverse typings (#14648)

* export removeProperties options

* traverse

* fix other packages typings

* fix gatherNodeParts

* downgrade to-fast-properties to v3

* fix typo

* babel-core/normalize-file

* add ignore comment property used by flow plugin

* refine getLastStatement typings

* fix babel-standalone rebase typing error

* fix assert.rejects polyfill

* simplify _param typings

* supress Babel 7 AST errors

* loosen defineType typings

The AST typings are generated from defineType calls, so defineType should accept a string as type.

* restore legacy code

* Suppress data/core-js-compat importing errors

Co-authored-by: Nicolò Ribaudo <nicolo.ribaudo@gmail.com>
Co-authored-by: liuxingbaoyu <30521560+liuxingbaoyu@users.noreply.github.com>
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Sep 21, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants