Type checking babel-types scripts #17494
Conversation
JLHwung
added
the
PR: Internal 🏠
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/59898 |
| return indexA - indexB; | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
These 3 methods are extracted to fieldHelpers, shared by the ast-types builders.
| return Array.isArray(ret) && ret.length === 1 && ret[0] === "any" | ||
| ? stringifyValidator(validator.chainOf[0], nodePrefix) | ||
| : ret; | ||
| if ("chainOf" in validator) { |
There was a problem hiding this comment.
Previously the chainOf depends on the special return value ["any"] for general validator ValidatorImpl, here we always return any for such validators and then extract the first meaningful type when iterating from the end to start.
|
commit: |
| shapeKey + | ||
| (isOptional ? "?: " : ": ") + | ||
| stringifyValidator(propertyDefinition.validate) | ||
| stringifyValidator(propertyDefinition.validate, nodePrefix) |
There was a problem hiding this comment.
This is a fix. nodePrefix should be passed through all stringifyValidator calls, although we don't have shapeOf({ a: assertOfNodeType("...") }) usage yet.
nicolo-ribaudo
left a comment
nicolo-ribaudo
left a comment
There was a problem hiding this comment.
Eventually we'll be able to just rewrite these scripts to TS, since if I remember correctly we don't run them on old Node.js versions.
Yes, they are build scripts so probably safe to do so. Attached the converted TS. |
JLHwung
force-pushed
the
type-checking-babel-types-scripts
branch
from
057efe7 to
6f737f9
Compare
| * @param {Validator | undefined} validator | ||
| * @returns {string[]} |
There was a problem hiding this comment.
Since we are already using ts, can we remove these JSDOC type definitions?
There was a problem hiding this comment.
The JSdoc provides more information than type definitions. It is a convenient place for examples, brief summary of the method or the parameter. So I think it'd better to keep it as-is.
There was a problem hiding this comment.
Only removing the types should work, since they're redundant.
If you're using eslint-plugin-jsdoc, I have the following near the top of the typescript-specific eslint config for my project:
{
'jsdoc/no-types': 'error', // Parameters should get their type info from TypeScript.
'jsdoc/no-undefined-types': 'off', // TypeScript already checks whether types are known.
'jsdoc/require-param-type': 'off', // Param types should be set using TypeScript.
'jsdoc/require-property-type': 'off', // Property types should be set using TypeScript.
'jsdoc/require-returns-type': 'off', // Return type should be set using TypeScript.
'jsdoc/require-template': 'off', // Templates are set using TypeScript.
}Probably not needed if you're using the flat/recommended-typescript config or similar though.
There was a problem hiding this comment.
Yes, I meant to also remove type definitions that are identical to TS types, while keeping the comments. :)
There was a problem hiding this comment.
Thanks, I will add eslint-plugin-jsdoc in a new PR so that the changeset here will not be bloated.
nicolo-ribaudo
left a comment
nicolo-ribaudo
left a comment
There was a problem hiding this comment.
(pending fixing/ignoring the lint error)
github-actions
bot
added
the
outdated
5 participants
In this PR we enabled the@ts-checkon all babel-types scripts.In this PR we converted babel-types scripts to TS.
The field
Validatortype is also revised such that it can be distributed to a series of sub types given specific validator kind such asoneOforoneOfNodeTypes. Thanks to the improved types, we can get rid of a few type castings or suppressed errors in our AST definitions.The
toFunctionNamehelper is replaced by theformatBuilderNamesince they serve for the same builder name calculation purpose.