-
Notifications
You must be signed in to change notification settings - Fork 905
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
improve acorn-walk types, add ".d.ts tests" using
tsd
This makes two changes to the TS types for acorn-walk.
**Using `AnyNode` in more places**
We currently have the discriminated union `AnyNode`, who's getting used in the functions who accept a visitor object. It enables us to receive the actual Node type as an argument to our visitor function:
```
walk.simple(ast, {
Literal: (n) => {
// works because our visitor function receives a node of type:
// Extract<acorn.AnyNode, { type: "Literal" }>
console.log(n.value)
}
})
```
If we weren't using the discriminated union, `n` would be of type `Node`, and when we try to access `n.value`, we'd get the TS error `TS2339: Property value does not exist on type Node`.
With this change, we now use `AnyNode` in the other places where we receive a Node in a callback, like `findNodeAt`:
```
walk.findNodeAt(ast, null, null, (_, n) => {
return n.type === "Literal" && n.raw?.includes("someSubstring")
})
```
With the discriminated union, once we confirm `n.type === "Literal"`, TS can "know" that `n` is a `Literal`, and we can access its fields appropriately.
This doesn't change `Node` to `AnyNode` in the parameters to the walker functions themselves, since we don't use TS in the library itself, so we have no need to disambiguate between input Nodes. If we pass e.g. a `BlockStatement` as the first argument to `simple`, that's fine for TS because a `BlockStatement` is a `Node`. I don't think it would hurt anything to change those occurrences to `AnyNode` too, though.
**Cleaning up some duplication**
The visitor object types previously contained a little duplication because visitor objects can contain either Node type names (like `Literal`) or Aggregate type names (like `Pattern`) as keys. Previously, we used an intersection type (`{} & {}`) to account for the Node/Aggregate cases, but now there's a single conditional type who we can share between the visitor object types. Hopefully giving names to all the different parts makes things clearer too.
Also, with `NodeOrAggregateTypeName` representing both sets of type names, we can make the typing a little more accurate for the functions where we receive the type name in a callback.
**Adding type definition tests**
As part of making the change, I wrote some "type definition tests" using the [`tsd` library](https://github.com/tsdjs/tsd), where we can assert things like "when we write a visitor function for the Node type `Literal`, the walker function will call it with a `Literal` as the first argument."
I found them helpful in giving me some confidence while making the changes (just as regular tests do), so I feel like they're worth having if we ever need to change the types. We can run the same test with both the `walk.d.ts` before and after the changes, to see which new scenarios are accounted for with the changes.
I excluded the test file from ESLint; we could add it, it would just require us adding the [TS ESLint plugin](https://typescript-eslint.io/getting-started/legacy-eslint-setup/), but I wasn't sure if we wanted that (I noticed the `test` directory is also currently excluded from ESLint).
We could also make the test run as part of CI. It runs with `npx tsd` in the acorn-walk directory and uses the test in the `test-d` directory to test `dist/walk.d.ts` by default.- Loading branch information
Showing
4 changed files
with
245 additions
and
30 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,202 @@ | ||
| import { | ||
| expectAssignable, | ||
| expectError, | ||
| expectNotAssignable, | ||
| expectType | ||
| } from "tsd" | ||
|
|
||
| import * as acorn from "acorn" | ||
| import * as walk from ".." | ||
|
|
||
| type simpleParams = Parameters<typeof walk.simple> | ||
| type ancestorParams = Parameters<typeof walk.ancestor> | ||
| type recursiveParams = Parameters<typeof walk.recursive> | ||
| type makeParams = Parameters<typeof walk.make> | ||
| type fullParams = Parameters<typeof walk.full> | ||
| type fullAncestorParams = Parameters<typeof walk.fullAncestor> | ||
| type findNodeAtParams = Parameters<typeof walk.findNodeAt> | ||
| type findNodeAroundParams = Parameters<typeof walk.findNodeAround> | ||
| type findNodeAfterParams = Parameters<typeof walk.findNodeAfter> | ||
| type findNodeBeforeParams = Parameters<typeof walk.findNodeBefore> | ||
|
|
||
| const ast = acorn.parse("", {ecmaVersion: "latest"}) | ||
|
|
||
| const v = () => {} | ||
|
|
||
| /* | ||
| Functions who accept visitor objects or "walker objects" should allow both Node | ||
| type names and Aggregate type names in the object. | ||
| */ | ||
| expectAssignable<simpleParams>([ast, | ||
| {Super: v, Class: v}, | ||
| {Super: v, Class: v} | ||
| ]) | ||
|
|
||
| expectAssignable<ancestorParams>([ast, | ||
| {Super: v, Class: v}, | ||
| {Super: v, Class: v} | ||
| ]) | ||
|
|
||
| expectAssignable<recursiveParams>([ast, null, | ||
| {Super: v, Class: v}, | ||
| {Super: v, Class: v} | ||
| ]) | ||
|
|
||
| expectAssignable<makeParams>([ | ||
| {Super: v, Class: v}, | ||
| {Super: v, Class: v} | ||
| ]) | ||
|
|
||
| expectAssignable<fullParams>([ast, v, | ||
| {Super: v, Class: v} | ||
| ]) | ||
|
|
||
| expectAssignable<fullAncestorParams>([ast, v, | ||
| {Super: v, Class: v} | ||
| ]) | ||
|
|
||
| expectAssignable<findNodeAtParams>([ast, null, null, "Super", | ||
| {Super: v, Class: v} | ||
| ]) | ||
|
|
||
| expectAssignable<findNodeAroundParams>([ast, null, "Super", | ||
| {Super: v, Class: v} | ||
| ]) | ||
|
|
||
| expectAssignable<findNodeAfterParams>([ast, null, "Super", | ||
| {Super: v, Class: v} | ||
| ]) | ||
|
|
||
| expectAssignable<findNodeBeforeParams>([ast, null, "Super", | ||
| {Super: v, Class: v} | ||
| ]) | ||
|
|
||
| /* | ||
| Functions who accept visitor objects or "walker objects" should not allow | ||
| additional properties who are not valid Node/Aggregate type names | ||
| */ | ||
| expectNotAssignable<simpleParams>([ast, {NotANode: v}]) | ||
| expectNotAssignable<simpleParams>([ast, {}, {NotANode: v}]) | ||
|
|
||
| expectNotAssignable<ancestorParams>([ast, {NotANode: v}]) | ||
| expectNotAssignable<ancestorParams>([ast, {}, {NotANode: v}]) | ||
|
|
||
| expectNotAssignable<recursiveParams>([ast, null, {NotANode: v}]) | ||
| expectNotAssignable<recursiveParams>([ast, null, {}, {NotANode: v}]) | ||
|
|
||
| expectNotAssignable<makeParams>([{NotANode: v}]) | ||
| expectNotAssignable<makeParams>([{}, {NotANode: v}]) | ||
|
|
||
| expectNotAssignable<fullParams>([ast, v, {NotANode: v}]) | ||
|
|
||
| expectNotAssignable<fullAncestorParams>([ast, v, {NotANode: v}]) | ||
|
|
||
| expectNotAssignable<findNodeAtParams>([ast, null, null, "Super", | ||
| {NotANode: v} | ||
| ]) | ||
|
|
||
| expectNotAssignable<findNodeAroundParams>([ast, null, "Super", | ||
| {NotANode: v} | ||
| ]) | ||
|
|
||
| expectNotAssignable<findNodeAfterParams>([ast, null, "Super", | ||
| {NotANode: v} | ||
| ]) | ||
|
|
||
| expectNotAssignable<findNodeBeforeParams>([ast, null, "Super", | ||
| {NotANode: v} | ||
| ]) | ||
|
|
||
| /* | ||
| When we supply a visitor function to a walker function, and the walker function | ||
| calls us with a given Node, the Node should be of the type we specified as the | ||
| visitor function name. | ||
| If our visitor function is for an Aggregate, the Node we receive should be a | ||
| member of a discriminated union, who allows us to determine the actual type | ||
| of the Node. | ||
| */ | ||
| walk.simple(ast, { | ||
| Super: (node) => { expectType<acorn.Super>(node) }, | ||
| Pattern: (node) => { | ||
| expectType<acorn.Pattern>(node) | ||
| if (node.type === "Identifier") { expectType<acorn.Identifier>(node) } | ||
| } | ||
| }) | ||
|
|
||
| walk.ancestor(ast, { | ||
| Super: (node) => { expectType<acorn.Super>(node) }, | ||
| Pattern: (node) => { | ||
| expectType<acorn.Pattern>(node) | ||
| if (node.type === "Identifier") { expectType<acorn.Identifier>(node) } | ||
| } | ||
| }) | ||
|
|
||
| walk.recursive(ast, null, { | ||
| Super: (node) => { expectType<acorn.Super>(node) }, | ||
| Pattern: (node) => { | ||
| expectType<acorn.Pattern>(node) | ||
| if (node.type === "Identifier") { expectType<acorn.Identifier>(node) } | ||
| } | ||
| }) | ||
|
|
||
| /* | ||
| Whenever we receive a Node as a parameter to a callback, it should be a member | ||
| of a discriminated union, who allows us to determine the actual type of the Node. | ||
| */ | ||
| walk.ancestor(ast, { | ||
| Super: (_node, _state, ancestors) => { | ||
| if (ancestors[0].type === "Super") { expectType<acorn.Super>(ancestors[0]) } | ||
| }, | ||
| }) | ||
|
|
||
| walk.full(ast, (node) => { | ||
| if (node.type === "Super") { expectType<acorn.Super>(node) } | ||
| }) | ||
|
|
||
| walk.fullAncestor(ast, (node, _, ancestors) => { | ||
| if (node.type === "Super") { expectType<acorn.Super>(node) } | ||
| if (ancestors[0].type === "Super") { expectType<acorn.Super>(ancestors[0]) } | ||
| }) | ||
|
|
||
| walk.findNodeAt(ast, null, null, (_, node) => { | ||
| if (node.type === "Super") { expectType<acorn.Super>(node) } | ||
| return true; | ||
| }) | ||
|
|
||
| /* | ||
| Whenever we receive a type name as a parameter to a callback, it should be a | ||
| valid Node/Aggregate type name | ||
| */ | ||
| walk.full(ast, (_node, _state, type) => { | ||
| expectAssignable<typeof type>("Super") | ||
| expectAssignable<typeof type>("Pattern") | ||
| expectNotAssignable<typeof type>("NotANode") | ||
| }) | ||
|
|
||
| walk.fullAncestor(ast, (_node, _state, _ancestors, type) => { | ||
| expectAssignable<typeof type>("Super") | ||
| expectAssignable<typeof type>("Pattern") | ||
| expectNotAssignable<typeof type>("NotANode") | ||
| }) | ||
|
|
||
| walk.findNodeAt(ast, null, null, (type) => { | ||
| expectAssignable<typeof type>("Super") | ||
| expectAssignable<typeof type>("Pattern") | ||
| expectNotAssignable<typeof type>("NotANode") | ||
| return true; | ||
| }) | ||
|
|
||
| /* | ||
| findNodeAt should reject an input string "type" who is not a valid | ||
| Node/Aggregate type name | ||
| */ | ||
| expectError(walk.findNodeAt(ast, null, null, "NotANode")) | ||
| expectError(walk.findNodeAround(ast, null, "NotANode")) | ||
|
|
||
| /* | ||
| findNodeAt should return a Node who's a member of a discriminated union, who | ||
| allows us to determine the actual type of the Node. | ||
| */ | ||
| const found = walk.findNodeAt(ast, null, null, () => true) | ||
| if (found?.node.type === "Literal") { expectType<acorn.Literal>(found?.node) } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters