Improved Flow Types for babel/types #7
Conversation
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
There was a problem hiding this comment.
It seems to me that the major problem that this RFC is addressing is that %check refinements only work for bare function calls and not for methods, so these things don't work:
types.isIdentifier(node);
path.isIdentifier();While looking at this problem in the past, I found facebook/flow#7735 which makes Flow do the correct thing without us needing to modify our types to workaround this missing feature.
|
|
||
| # Drawbacks | ||
|
|
||
| * Braking Change: Applications that use the existing flow types and rely on the `types.is*` methods would have to migrate to `node.type ===` checks. |
There was a problem hiding this comment.
This is a big usability regression. While it's easy to replace things like types.isIdentifier(), manually inlining functions like types.isExpression() or types.isLoop() is a mess.
I wouldn't be comfortable with dropping these %checks utilities.
There was a problem hiding this comment.
I agree that this is a regression and I don't know how widely %checks is used in the wild. I want to clarify that writing out the %checks predicate is only allowed for function declaration (declare function) but not when defining a function. Flow automatically infers the predicate if it's a function definition. Function declaration are mostly used in declaration files.
There was a problem hiding this comment.
It seems to work with union types 😄
There was a problem hiding this comment.
Yes, %checks with .type works perfectly. That's what this PR is making use of. The only thing where it gets messy, as you pointed out, is that writing declare function isLiteral(t) % checks t.type === 'StringLiteral' || t.type === 'NumericLiteral' ... is somewhat lengthy. I first thought that it was possible in the past to write declare function isLiteralI(t) %checks t instanceof BabelNodeLiteral but that wasn't the case since there was no BabelNodeLiteral type.
So the only regressions are that manual %checks must be rewritten and that the node types are no longer exported as value which has the implications that:
- Nodes must be imported using
import type, - Node types must be extended using the spread operator instead of
extends(should be a very rare use case) - Any other value reference to node types is (correctly) illegal
There was a problem hiding this comment.
As far as I understand, this RFC doesn't mention that we can still use %checks with type. It says that it isn't possible to still use the is* methods for type refinements:
Braking Change: Applications that use the existing flow types and rely on the types.is* methods would have to migrate to node.type === checks.
I see that your PR already keeps the %checks in the generated type annotations: could you update this RFC text to match that behavior? You can also write that this is an improvement, since things like isPattern() will now help refining.
PS. You can add me as a champion in the RFC header.
There was a problem hiding this comment.
Oh sorry. My fault. I'll update the RFC. It's a relic from an earlier version of the RFC.
|
|
||
| # Open questions | ||
|
|
||
| * Should the global `BabelNode*` exports be replaced by module level exports without the `BabelNode` prefix to better align with the type script types or should the definition file export both until the next major version. |
There was a problem hiding this comment.
We can both use the global BabelNode* version and normal named exports for now, and then remove the global version in Babel 8.
|
Thanks @nicolo-ribaudo for looking into this RFC. The main motivation is to create a typing that allows a natural usage of the babel-types. E.g. I would expect that flow correctly refines a node to a string literal when writing The second motivation was to remove unsoundness from the type definition. E.g. two examples
I further believe that we shouldn't wait for a stale Flow PR (no update in 1.5 years) to improve the devx for people working with babel (I've a local version of babel-traverse). |
|
AFAICT,
I've rebased that PR, let's see what happens... facebook/flow#8525 |
|
thank you @mischnic for rebasing the PR. Support for some for of refinements for The proposed PR allows to refine |
| ## Create a transform script | ||
|
|
||
| Create a babel plugin that rewrites the `%checks` refinements and changes the imports of node types from values to types. |
There was a problem hiding this comment.
I don't think that this will be need in practice. Personally I expect that the number of people manually writing %checks for Babel types in custom functions is close to zero, and I would consider this PR strictly as a bugfix (i.e. mark nodes not as classes).
Code like this is invalid in flow:
function check(x: A | B): %checks (x instanceof A) {
return x.type === "a";
}
|
Any updates on this? Typing the |
View Rendered Text
PR: babel/babel#12135