-
-
Notifications
You must be signed in to change notification settings - Fork 5.8k
[Babel 8] Better node type definitions for computed
#17500
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
Conversation
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/59924 |
|
@liuxingbaoyu Could you rebase? Thank you. |
nicolo-ribaudo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice!
| NODE_FIELDS, | ||
| NODE_UNION_SHAPES__PRIVATE, | ||
| toBindingIdentifierName, | ||
| } = _t as typeof import("@babel/types"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we instead say in tsconfig.json that imoprts to that lib/index.js should be mapped to the TS files for the type definitions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried, but without success.
0775ba1 to
995036a
Compare
Co-authored-by: Nicolò Ribaudo <[email protected]>
5882531 to
d5ed143
Compare
d5ed143 to
8bc58e4
Compare
|
commit: |
|
|
||
| /** | ||
| * Generate the builder arguments for a given node type. | ||
| * @param type AST Node type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a short description for the declare parameter?
JLHwung
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job!
| // Future / annoying TODO: | ||
| // MemberExpression.property, ObjectProperty.key and ObjectMethod.key need special cases; either: | ||
| // - convert the declaration to chain() like ClassProperty.key and ClassMethod.key, | ||
| // - declare an alias type for valid keys, detect the case and reuse it here, | ||
| // - declare a disjoint union with, for example, ObjectPropertyBase, | ||
| // ObjectPropertyLiteralKey and ObjectPropertyComputedKey, and declare ObjectProperty | ||
| // as "ObjectPropertyBase & (ObjectPropertyLiteralKey | ObjectPropertyComputedKey)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Has this todo item been done in this PR? Ditto for line 46.
|
The CI failure is related and is being investigated! |
c750d22 to
1072c2d
Compare
|
Due to #11384, I ended up not using |
computedcomputed
| return withStartEnd({ | ||
| type: "Identifier", | ||
| - name: node.getText(), | ||
| + name: text.startsWith(`"`) ? JSON.parse(text) : text, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we mark the field as @internal with TSDoc, and make it not appear in the generated .d.ts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It worked, thanks!
nicolo-ribaudo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(don't forget to make this babel-8-only)
| /** | ||
| * @internal | ||
| */ | ||
| export { NODE_UNION_SHAPES__PRIVATE as "PRIVATE!!! NODE_UNION_SHAPES" }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the @internal works, maybe we don't need the export name hack and can just keep the NODE_UNION_SHAPES__PRIVATE name?
I did so in |
Co-authored-by: Huáng Jùnliàng <[email protected]>
Fixes #1, Fixes #2Only the last commit is new.
Once the initial review is complete, I'll move it to Babel 8 only.