Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Summary: This diff adds a more complete declaration file for `babel/types` and parts of `NodePath` from `babel/traverse`. ## Motivation * Get better intelli-sense * Unleash flow's super power and allow it to catch errors at compile time. This will also allow us to catch not yet handled cases after upgrading `babel`. E.g. the support for `SpreadElements`. * More explicit types in our APIs that are part of Metro's documentation * Delete repetitive and incomplete/incorrect typings. E.g the code base has multiple incomplete definitions of the Visitor interface ## Typing ### `babel/types` This diff uses the `babel/types` proposed in the [RFC](babel/rfcs#7) submitted to the babel repository. The PR hasn't been reviewed nor accepted. It's, therefore, likely that further changes are needed. We can adopt to any changes, if any, on our own paces since we have a custom `babel/types` declaration file today which we'll hopefully able to delete once the RFC has been accepted and the PR's merged and released. We also have the option to use our own fork of the typing generation script if needed. ### `babel/traverse` There are no official nor in-offical existing types for `babel/traverse` today. My goal is to submit our types to flow-typed once the RFC is merged and the types are more complete. The definitions are based on the Typescript definitions. ## Shortcomings ### No `%checks` support on methods Flow doesn't support `%checks` refinements on object methods. That also applies if a function is invoked on a method, e.g. `t.isIdentifier(node)` will not refine `node` to an `Identifier`. This requires us to destructure the `is*` methods to get type refinement. ```js if (t.isIdentifier(node)) { // ^^ Doesn't refine because the function invoked as a method console.log(node.name); } const {isIdentifier} = t; if (isIdentifier(node)) { console.log(node.name); // works fine } ``` ### `NodePath` Refinement Flow doesn't support `%checks` refinements on methods as mentioned in the previous section. That means that the `NodePath.is*` methods cannot be correctly typed to refine `NodePath` to a more specific type. It's further not possible to refine a `NodePath` by checking on the type of the node. E.g. ``` function test(path: NodePath<Node>): void { if (path.node.type === "Identifier") { const identifier: Identifier = path.node; // <--- works const identifierPath: NodePath<Identifier> = path; <-- Doesn't work. } } ``` See [Post](https://fb.workplace.com/groups/flow/permalink/4458059784242602/) for the reasons. This is unfortunate because there's no way to cast `NodePath<T>` to `NodePath<S>` (to my knowledge) except casting over any which is unsafe. There were only a few places in the code base today where this was needed. I'll try to rethink this when working on a more complete typing for `babel/traverse`. ### `NodePath.node` is marked read-only I marked `NodePath.node` as read-only such that a more specific `NodePath` can be assigned to a more generic `NodePath`. E.g. that the following assignment is supported ``` function getPathKey(path: NodePath<>): string { return path.key; } function test(path: NodePath<CallExpression>): void { const key = getPathKey(path); ^^^^^ // Casts NodePath<CallExpression> to NodePath<> } ``` However, the `node` isn't read-only in reality, e.g. it can be replaced using `NodePath.replaceWith(somethingElse)`. A sound definition of `NodePath` would define `node` as non-generic. However, that makes it extremelly cumbersome to work with `Node` e.g. in visitors because `invariant` checks would been needed in all visitor method if the accepted node is of the expected type so that flow refines the node (Flow would invalidate the refinement after every function / method calls which will make it nearly impossible). I believe that this unsoundness is acceptable and it's still a huge improvement over not having any types at all. ### `NodePath.get` isn't generic `NodePath.get` is an interesting method because it returns either a `NodePath<TNode>` or `Array<NodePath<TNode>>` depending on whatever the field of the passed in key is an array type or not. It's, unfortunately, not possible to type this method as generic today because of a [bug in flow](https://fb.workplace.com/groups/flow/permalink/4460750363973544/). This requires some additional invariants in the code to help flow understand what's going on. ## Summary I believe that the added types will help us to catch bugs when writing new code and in the existing code base. It will further help us to know what new cases we need to handle when upgrading Babel to a new version. Having better intellisense further helps during development (e.g. I spend hours wondering why `template()` produced incorrect code when I passed a string placeholder instead of a node. However, it comes at the cost that `babel/types` cannot be exactly typed which means that some additional invariants are needed to make flow happy and it can sometimes mean that we'll have to cast over `any`. I believe that this additional cost is worth taking in regards of what the additional typing gives us in return. Reviewed By: cpojer Differential Revision: D24117447 fbshipit-source-id: d00440bfeaccd827b2cb412e50bd17d54f96636d
- Loading branch information