Enable strictNullChecks for traverse#17499
Conversation
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/60040 |
| skipKeys: Record<string, boolean> | null = null; | ||
| parentPath: NodePath_Final = null; | ||
| parentPath: NodePath_Final | null = null; | ||
| container: t.Node | Array<t.Node> | null = null; | ||
| listKey: string | null = null; | ||
| listKey: string | null | undefined = null; | ||
| key: string | number | null = null; | ||
| node: t.Node | null = null; | ||
| type: t.Node["type"] | null = null; |
There was a problem hiding this comment.
Perhaps we could define more properties as non-null, such as container, like we did with parentPath.
There was a problem hiding this comment.
I'm a bit hesitant to do the same with scope.parent.
| interface NodePath< | ||
| N extends t.Node | null, | ||
| T extends t.Node["type"] = NonNullable<N>["type"], | ||
| > extends InstanceType<typeof NodePath_Final>, |
There was a problem hiding this comment.
NodePath<null> was introduced here, but it was not used more in path.get because it would be very cumbersome to use if we used strict null and non-null.
Suggestions welcome!
There was a problem hiding this comment.
It seems the ecosystem has moved to a stricter null, I'll double-check.
| self.replaceWith({ | ||
| type: "BlockStatement", | ||
| directives: [], | ||
| body: [], | ||
| } as t.BlockStatement); | ||
| } satisfies t.BlockStatement); | ||
| return true; |
There was a problem hiding this comment.
This is a very good fix.
There was a problem hiding this comment.
Right, though looking back I think we could just use the blockStatement builder, since the @babel/types is already a dependency of @babel/traverse.
|
commit: |
bf1c9f0 to
e0bcaa6
Compare
JLHwung
left a comment
There was a problem hiding this comment.
This PR looks good to me in general. Nice work.
| binding: Binding, | ||
| path: NodePath, | ||
| name: string, | ||
| name: string | undefined, |
There was a problem hiding this comment.
The name here is passed from a name property of an Identifier, I think it should be always a string unless the AST is malformed.
Can we remove the optional marker here?
| if (types.length) { | ||
| return { | ||
| // @ts-expect-error FIXME: may return undefined | ||
| typeAnnotation: createUnionType(types), |
There was a problem hiding this comment.
The createUnionType will return something as long as types are either all Flow types or TS types. The parser will not mix flow types with ts types, so if there are any outliers, they should be considered malformed AST. In this case can we use the definite marker here?
| // We also move past any constant violations. | ||
| for (const violationPath of binding.constantViolations) { | ||
| // @ts-expect-error comparing undefined and number | ||
| if (this.getAttachmentParentForPath(violationPath).key > path.key) { |
There was a problem hiding this comment.
The getAttachmentParentForPath now always return a NodePath, does TS throw an error because we are comparing string to number?
There was a problem hiding this comment.
It seems that it is because key may be null.
bceb22f to
d423e2f
Compare
|
I pushed |
|
I'm marking this as a bug fix because this fixes compatibility issues with |
| block!: t.Pattern | t.Scopable; | ||
|
|
||
| inited; | ||
| declare inited; |
There was a problem hiding this comment.
Can we avoid declare here? We want them to be defined as own properties.
ce2662e to
2b67663
Compare
Fixes #1, Fixes #2