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

Provide plugin/preset typings from plugin-utils #14499

Merged
merged 22 commits into from Apr 29, 2022

Conversation

JLHwung
Copy link
Contributor

@JLHwung JLHwung commented Apr 27, 2022

Q                       A
Fixed Issues? Official Babel plugins finally have typing inference without manual typing annotations in most cases, also fixing bugs in partial-application caught by the type checker
Patch: Bug Fix? Y
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

In this PR we provides typings from declare / declarePreset in @babel/helper-plugin-utils, which is used in every official Babel plugins.

For most plugins with type-only visitors, type inference works without manual annotations.

We introduce a declarePreset method for presets usage. I didn't figure out how to merge typing hints with declare so I end up with a new method.

@JLHwung JLHwung added the PR: Internal 🏠 A type of pull request used for our changelog categories label Apr 27, 2022
@@ -12,7 +14,9 @@ export default declare(api => {
name: "bugfix-v8-spread-parameters-in-optional-chaining",

visitor: {
"OptionalCallExpression|OptionalMemberExpression"(path) {
"OptionalCallExpression|OptionalMemberExpression"(
path: NodePath<t.OptionalCallExpression | t.OptionalMemberExpression>,
Copy link
Contributor Author

@JLHwung JLHwung Apr 27, 2022

Choose a reason for hiding this comment

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

We still have to manually annotate the input of the multiple-type selection visitors.

rejectId?: t.Identifier;
}

export default declare<State>((api, options: Options) => {
Copy link
Contributor Author

@JLHwung JLHwung Apr 27, 2022

Choose a reason for hiding this comment

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

A plugin can declare its own State definition, the state will be available as the second callback param of NodePath visitors.

However I have to export the State interface otherwise TS will complain at preset-env/src/available-plugins:

TS4082: Default export of the module has or is using private name 'State'.

74 export default {
~~~~~~~~~~~~~~~~
75 "bugfix/transform-async-arrows-in-class": () => bugfixAsyncArrowsInClass,
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
...
149 "transform-unicode-regex": () => transformUnicodeRegex,
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
150 };
~~

I don't think the State should be exported, suggestions are welcome.

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo Apr 27, 2022

Choose a reason for hiding this comment

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

Does TS also complain if we define it inline? i.e.

export default declare<{
  requireId?: t.Identifier;
  resolveId?: t.Identifier;
  rejectId?: t.Identifier;
}>((api, options: Options) => {

and if we use type instead of interface?

Copy link
Contributor Author

@JLHwung JLHwung Apr 28, 2022

Choose a reason for hiding this comment

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

Using type works as intended, thanks!

@JLHwung JLHwung force-pushed the expose-plugin-preset-api-types branch from afb6f64 to 53453de Compare Apr 27, 2022
@@ -46,7 +52,7 @@ export default declare((api, options) => {
return value;
}

function getBuiltIn(name, path, programPath) {
function getBuiltIn(name, programPath) {
Copy link
Contributor Author

@JLHwung JLHwung Apr 27, 2022

Choose a reason for hiding this comment

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

The path is not used.

@@ -7,7 +7,7 @@ export default declare(api => {
name: "syntax-import-assertions",

manipulateOptions(opts, parserOpts) {
parserOpts.plugins.push(["importAssertions"]);
parserOpts.plugins.push("importAssertions");
Copy link
Contributor Author

@JLHwung JLHwung Apr 27, 2022

Choose a reason for hiding this comment

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

They are equivalent, but the type-checker favors the latter form, which is also more common.

@babel-bot
Copy link
Collaborator

babel-bot commented Apr 27, 2022

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

@@ -36,12 +43,11 @@ export default declare(api => {
}

const { node } = func;
if (t.isMethod(node)) {
Copy link
Contributor Author

@JLHwung JLHwung Apr 27, 2022

Choose a reason for hiding this comment

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

A method must not have id so this step can be advanced, which also helps narrowing down the types.

@JLHwung JLHwung force-pushed the expose-plugin-preset-api-types branch from 53453de to c20d40b Compare Apr 27, 2022
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

This is awesome!

@@ -24,7 +24,7 @@ type CallerFactory = (
extractor: (callerMetadata: CallerMetadata | void) => unknown,
) => SimpleType;
type TargetsFunction = () => Targets;
type AssumptionFunction = (name: string) => boolean | void;
type AssumptionFunction = (name: AssumptionName) => boolean;
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo Apr 27, 2022

Choose a reason for hiding this comment

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

Technically a plugin could be used with an old @babel/core version that doesn't know about a new assumption and thus returns undefined.

We could annotate this as (name: AssumptionName) => boolean | void: it's fine (and more helpful) if the parameter is more restrictive, but the return type should cover all the possibilities to avoid possible bugs caused by assuming that it's always a boolean.

Copy link
Contributor Author

@JLHwung JLHwung Apr 28, 2022

Choose a reason for hiding this comment

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

If we stick to the principle that an assumption always introduce certain non-spec constraints, I would not oppose to make it always return false for unknown assumptions. Otherwise plugins has to cast .assumption(name) to boolean by themselves.

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo Apr 29, 2022

Choose a reason for hiding this comment

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

Well, there is at least noNewArrow that does api.assumption("noNewArrow") ?? true, so "just return false" doesn't work.

Copy link
Contributor Author

@JLHwung JLHwung Apr 29, 2022

Choose a reason for hiding this comment

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

Don't we want to default it to false in Babel 8? 🙂

packages/babel-core/src/config/validation/plugins.ts Outdated Show resolved Hide resolved
packages/babel-helper-plugin-utils/src/index.ts Outdated Show resolved Hide resolved
export default declare((api, opt: Options) => {
api.assertVersion(7);
const { types: t, template } = api;
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo Apr 27, 2022

Choose a reason for hiding this comment

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

Q: Does TS complain about destructuring?

Copy link
Contributor Author

@JLHwung JLHwung Apr 27, 2022

Choose a reason for hiding this comment

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

Nope. TS correctly picks up typeof import ("@babel/types") for t.

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo Apr 27, 2022

Choose a reason for hiding this comment

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

I didn't select enough lines when commenting! The original code was export default declare(({ assertVersion, types: t, template }`, and I wondered if the destructuring directly in the param made it complain.

Copy link
Contributor Author

@JLHwung JLHwung Apr 28, 2022

Choose a reason for hiding this comment

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

destructuring directly in the param made it complain

Nope. I moved destructuring in param to body in order to avoid a wall of whitespace changes. In packages/babel-plugin-proposal-class-static-block/src/index.ts destructuring in param works as expected.

rejectId?: t.Identifier;
}

export default declare<State>((api, options: Options) => {
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo Apr 27, 2022

Choose a reason for hiding this comment

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

Does TS also complain if we define it inline? i.e.

export default declare<{
  requireId?: t.Identifier;
  resolveId?: t.Identifier;
  rejectId?: t.Identifier;
}>((api, options: Options) => {

and if we use type instead of interface?

@@ -7,8 +7,8 @@ import type * as t from "@babel/types";
export default declare(api => {
api.assertVersion(7);

const noDocumentAll = api.assumption("noDocumentAll");
const pureGetters = api.assumption("pureGetters");
const noDocumentAll = (api.assumption("noDocumentAll") ?? false) as boolean;
Copy link
Contributor Author

@JLHwung JLHwung Apr 28, 2022

Choose a reason for hiding this comment

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

This is microsoft/TypeScript#40359. TS currently can not narrow down (boolean | void) ?? boolean to boolean

@@ -99,7 +108,7 @@ export default declare((api, options, dirname) => {
}

if (has(options, "useBuiltIns")) {
if (options.useBuiltIns) {
if (options["useBuiltIns"]) {
Copy link
Contributor Author

@JLHwung JLHwung Apr 28, 2022

Choose a reason for hiding this comment

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

I didn't add useBuiltIns and polyfill to Options because they have been removed prior to Babel 7.

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

🚀

Copy link
Member

@existentialism existentialism left a comment

❤️

@nicolo-ribaudo nicolo-ribaudo merged commit c90add7 into babel:main Apr 29, 2022
36 checks passed
@nicolo-ribaudo nicolo-ribaudo deleted the expose-plugin-preset-api-types branch Apr 29, 2022
@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 Jul 30, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 30, 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 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

4 participants