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

Fix t.arrowFunctionExpression and t.functionExpression builders by adding all fields #12334

Closed
wants to merge 1 commit into from

Conversation

deificx
Copy link
Contributor

@deificx deificx commented Nov 8, 2020

Q                       A
Fixed Issues? Fixes #12319
Patch: Bug Fix? Yes
Major: Breaking Change? No
Minor: New Feature? No
Tests Added + Pass? Yes
Documentation PR Link N/A
Any Dependency Changes? No
License MIT

This fix adds the additional fields to the builder arrays for the definitions.
However, this does feel like a workaround, I also found this bug to apply to assignmentPattern, and there are probably more.
It looks like defineType in utils.js makes sure to add both builder and visitor fields to a node while the builder function in builder.js only checks the builder values.

Somewhat unrelated, but I can't figure out how to add types to function parameters.
t.tsParameterProperty looks like the wrong type to me, it looks like something you'd add to a class (ie: readonly property = "value").
The closest thing I can find for this is t.tsPropertySignature, but that looks like some thing for an interface (ie: key: type; note the trailing ;).

@codesandbox-ci
Copy link

codesandbox-ci bot commented Nov 8, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 56aa9b9:

Sandbox Source
babel-repl-custom-plugin Configuration
babel-plugin-multi-config Configuration

@babel-bot
Copy link
Collaborator

babel-bot commented Nov 8, 2020

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

@deificx
Copy link
Contributor Author

deificx commented Nov 8, 2020

I force pushed as I was branching from master, rebased from main and now I've broken a bunch of tests. So adding these properties to builder in definition is probably not the correct solution.

@deificx
Copy link
Contributor Author

deificx commented Nov 8, 2020

To answer my side note I guess t.identifier could be used, as that has a typeAnnotation, but t.identifier() only accepts 1 argument. And when trying to manually build it generator doesn't seem to do anything with it.

const g = require("@babel/generator").default;

console.log(
  g({
    name: "foo",
    optional: true,
    type: "Identifier",
    typeAnnotation: {
      type: "TSTypeAnnotation",
      typeAnnotation: {
        type: "TSStringKeyword",
      },
    },
  }).code
); // foo

I would expect something like foo?: string

@nicolo-ribaudo
Copy link
Member

@deificx As you may have noticed, we don't provide builder parameters for types in JS nodes. We only support parameters for JavaScript syntax and for JavaScript proposal.
If we started adding params for type annotations, we would have to somehow support both TS and Flow features at the same time.

The recommended way of achieving what you need is this:

Object.assign(t.arrowFunctionExpression(params, body, async), {
  returnType: yourReturnTypeNode,
});

Object.assign(t.identifier("foo"), {
  typeAnnotation: t.tsTypeAnnotation(t.tsStringKeyword())
});

@deificx
Copy link
Contributor Author

deificx commented Nov 10, 2020

...we don't provide builder parameters for types in JS nodes.

It looks like you used to, ie. identifier:

typeAnnotation: TypeAnnotation | TSTypeAnnotation | Noop (default: null)

Or is TypeAnnotation not the same as a FlowType?

Adding a more complete example here, mostly for my understanding, identifier with type annotation must be used in parameters for it to print the type annotation, because by itself it would only print the identifier:

const g = require("@babel/generator").default;
const t = require("@babel/types");

console.log(
  g({
    ...t.arrowFunctionExpression(
      [
        {
          ...t.identifier("foo"),
          typeAnnotation: t.tsTypeAnnotation(t.tsStringKeyword()),
        },
        {
          ...t.identifier("bar"),
          typeAnnotation: t.tsTypeAnnotation(t.tsBooleanKeyword()),
        },
      ],
      t.stringLiteral("")
    ),
    returnType: t.tsTypeAnnotation(t.tsStringKeyword()),
  }).code
); // (foo: string, bar: boolean): string => ""

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Feb 25, 2021

The recommended way is what is shown in #12334 (comment).

Thanks for opening the PR anyway, it's always good to check if old team decisions are still valid 🙏

Btw, you might also be interested in #9545.

@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 May 27, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 27, 2021
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.

t.arrowFunctionExpression not accepting arguments listed in documentation
3 participants