Convert repo to TypeScript; Auto-generate types for defs #300
Conversation
Don't force node type field to be the type name as a string literal. Only buildable types have a type field set to their type, as per https://github.com/brieb/ast-types/blob/94ff83cc3f9e971aecac0fdfa3c931f482e13a5a/lib/types.ts#L533 So, the previous types weren't true to what we'd encounter at runtime.
This allows us to use ASTNode as a type and narrow it based on the
`type` field. i.e.
```
function exampleFn(node: ASTNode) {
if (node.type === "Identifier") {
// node is narrowed to `Identifier` here
}
}
```
This pattern can also be seen in @babel/types
https://gist.github.com/brieb/e312c8f2565c4b2cde5996e1f648780e#file-index-d-ts-L44
When `string` is introduced to a union type, we are no longer able to narrow types because the type widens to `string` rather than a string literal union. Here is an example that demonstrates this: http://bit.ly/2QyiplJ Usage of the unioned node's type is an approach also used by `@babel/types`. https://gist.github.com/brieb/e312c8f2565c4b2cde5996e1f648780e#file-index-d-ts-L41 My guess is that this was added to address this situation.
test/ecmascript.ts
Outdated
| @@ -264,20 +268,23 @@ describe("whole-program validation", function() { | |||
| this.timeout(20000); | |||
|
|
|||
| // TODO(brieb): do we need these? | |||
There was a problem hiding this comment.
Looks like this is the only remaining TODO(brieb) in this PR. Before, we ran validateECMAScript on this repo's source files ("main.js", "lib/shared.js", etc). How should we address these test cases, now that the source code is .ts? Should we run it on the emitted files, add different more targeted test cases as new fixtures, remove them, something else?
There was a problem hiding this comment.
I think it would be useful to run namedTypes.Program.assert(ast, true) on some TypeScript ASTs, to assert that everything in the AST matches the expected types. Basically the same as shared.validateECMAScript, except it would only use the Babel TypeScript parser, not Esprima.
Actually, scratch that! It looks like we already do a deep assertion for every file we process in tests/typescript.ts here, which runs over the whole https://github.com/Microsoft/TypeScript/tree/master/src/compiler codebase.
In light of that, I'm pretty sure the commented-out tests are redundant, and we can just remove them.
This reverts commit 493d64f.
So, we can use type narrowing for fields of type `*Kind`.
For example,
```
function exampleFn(node: ASTNode) {
if (node.type === "CatchClause") {
if (node.param) {
if (node.param.type === "Identifier") {
// `node.param` is properly narrowed to `Identifier` here
console.log(node.param.name);
}
}
}
}
```
|
Cool - I think we're good to go as long as you think this change #300 (comment) / 2483cd2 makes sense! |
|
Makes sense to me! So… if export type ASTNode = K.PrintableKind | K.NodeKind | K.FileKind | ...If I'm understanding correctly, it's kinda neat how nodes and kinds are related via I noticed one small oddity: if (node.type === "SourceLocation") {
doSomething(node.start, node.end);
}since no However, that's just because I made |
The `SourceLocation` definition currently has `type:"SourceLocation"`,
even though `{ start, end, source }` location objects don't have a `type`
field when they're parsed. Same for the `Position` definition. This could
be misleading if you wrote code like
if (node.type === "SourceLocation") {
doSomething(node.start, node.end);
}
since no node would ever match that case.
Although I guess we could introduce a buildable node type that doesn't
have a `type` field, I think it's simpler to remove the builder functions
for SourceLocation and Position.
| .field("start", def("Position")) | ||
| .field("end", def("Position")) | ||
| .field("source", or(String, null), defaults["null"]); | ||
|
|
||
| def("Position") | ||
| .build("line", "column") |
There was a problem hiding this comment.
It's so great to see the immediate effects of a commit in the generated code. 💚
|
Merged! Woo! 🎉 Yes, it should be the case that the union of all kinds resolves to the same union as Here is the full list of supertypes without buildable subtypes (cases where |
|
Oh geez, we should probably stop loading |
|
Ok, I've published version 0.12.0 (with this PR included) to npm using the |
|
Awesome! Just updated benjamn/recast#554 to use that version. |
#300 (comment) The def/e4x.js definitions were inherited from the Mozilla Parser API, and represent a language feature (XML literals) that has long been abandoned. The Mozilla extensions (let statements, expression function bodies, &c.) have likewise become ancient history, not even still supported by Firefox.
#300 (comment) The def/e4x.js definitions were inherited from the Mozilla Parser API, and represent a language feature (XML literals) that has long been abandoned. The Mozilla extensions (let statements, expression function bodies, &c.) have likewise become ancient history, not even still supported by Firefox.
#300 (comment) The def/e4x.js definitions were inherited from the Mozilla Parser API, and represent a language feature (XML literals) that has long been abandoned. The Mozilla extensions (let statements, expression function bodies, &c.) have likewise become ancient history, not even still supported by Firefox.
Since benjamn#300 removed the legacy Mozilla and E4X AST definitions, the legacy `..` operator should be removed as well. Closes benjamn#238.
Following up from our twitter thread: https://twitter.com/benjamn/status/1063979819149475841
Started down the path of making d.ts files, but in combing through the code to make sure the types were correct, I increasingly felt like I'd have more confidence if we just converted the code to TS. Sounded like you were open to this idea from your twitter comment.
Goals:
module.exports =by using TypeScript'sexport =, so default export behavior doesn't changeTODO:
any), but are at least correct. We'll want to narrow these types over time.The results are pretty awesome!
Reviewers: @benjamn