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

Restructure virtual types validator #14799

Merged
merged 7 commits into from Jul 27, 2022

Conversation

JLHwung
Copy link
Contributor

@JLHwung JLHwung commented Jul 26, 2022

Q                       A
License MIT

This PR is extracted from #14179. The goal is to remove packages/babel-traverse/scripts/generators/virtual-types.js so that the build script doesn't rely on the internal virtualTypes. The virtual type validator typings are moved to path/lib/virtual-types-validator.ts, followed by implementations moved from path/lib/virtual-types.

I think it is fine to remove the extra build step because virtual types are not frequently updated compared to AST types.

@JLHwung JLHwung added PR: Internal 🏠 A type of pull request used for our changelog categories pkg: types labels Jul 26, 2022
@babel-bot
Copy link
Collaborator

babel-bot commented Jul 26, 2022

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

import type * as t from "@babel/types";
const { isCompatTag } = react;
import type { VirtualTypeAliases } from "./virtual-types";

Copy link
Contributor Author

@JLHwung JLHwung Jul 26, 2022

Choose a reason for hiding this comment

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

Before this PR I have to memoize the link from typing to implementation

path.isBindingIdentifier => virtualTypes["isBindingIdentifier"].checkType

Now they are placed together. However, the typings and implementations can not be merged because TS does not support this assertion in non-class functions, although they will be eventually injected to the NodePath prototype.

checkPath({ node }: NodePath<t.ForOfStatement>): boolean {
return node.await === true;
},
checkPath: path => path.isForAwaitStatement(),
Copy link
Member

Choose a reason for hiding this comment

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

Can we simplify Wrapper to remove checkPath? (I'm not sure if this is possible, I try to understand the codes but they are a bit complicated)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope. The checkPath applies additional selection logic based on Wrapper#types.

if (wrapper.checkPath(path)) {
return fn.apply(this, arguments);
}

Say if we have a ReferencedIdentifier visitor, Babel knows an Identifier could be a ReferencedIdentifier from Wrapper#types. Then it wraps the visitor so that the wrapped visitor will first run wrapper.checkPath, if the node is a ReferencedIdentifier, the wrapped visitor then applies the original visitor to the Identifier node, otherwise the visitor is inactivated.

Copy link
Member

Choose a reason for hiding this comment

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

Could we pass nodeType to

fns[type] = wrapCheck(wrapper, fns[type]);
, and then rewrite the wrapCheck function so that it doesn't rely anymore on checkPath? Every checkPath function just forwards the check to the .is* method with the same name as the virtual type.

function wrapCheck(nodeType: String, fn: Function) {
  const newFn = function (this: unknown, path: NodePath) {
    if (path[`is${nodeType}`]()) {
      return fn.apply(this, arguments);
    }
  };
  newFn.toString = () => fn.toString();
  return newFn;
}

Or, we can make checkPath just the check method name (as a string), to avoid the unnecessary intermediate function:

import * as virtualValidators from "./virtual-types-validator.ts"

export const ForAwaitStatement: Wrapper = {
  types: ["ForOfStatement"],
  checkPath: virtualValidators.isForAwaitStatement,
};
function wrapCheck(wrapper: Wrapper, fn: Function) {
  const newFn = function (this: unknown, path: NodePath) {
    if (wrapper.checkPath.call(path)) {
      return fn.apply(this, arguments);
    }
  };
  newFn.toString = () => fn.toString();
  return newFn;
}

Copy link
Member

@liuxingbaoyu liuxingbaoyu left a comment

Choose a reason for hiding this comment

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

This PR seems to expose more methods, but that's fine for me.

@JLHwung
Copy link
Contributor Author

JLHwung commented Jul 27, 2022

This PR does not expose any new methods. In current main, the virtual types validators are injected by

NodePath.prototype[`is${type}`] = function (opts?: any) {
// @ts-expect-error checkPath will throw when type is ExistentialTypeParam/NumericLiteralTypeAnnotation
return virtualType.checkPath(this, opts);
};

which invokes virtualTypes.checkPath() under the hood.

In this PR we move the implementation to the explicit NodePath#is{virtualType} method and have virtualTypes.checkPath called them instead, and we also eliminate the need for generating path.is{virtualType} typings from virtualTypes.

@JLHwung JLHwung merged commit 0416f16 into babel:main Jul 27, 2022
@JLHwung JLHwung deleted the refactor-virtual-type-validator branch July 27, 2022 17:00
@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 Oct 27, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 27, 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 pkg: types PR: Internal 🏠 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants