[WIP] Refactor 'visit' function

[IvanGoncharov]

I very frequently use visit function in my work. However I never fully understood how it's working especially since entire visitor.js lacks typing.

I managed to figure out some things and add typings to entire file (with the exception of a few any) and all tests are passing. But I stuck since I can't understand original intentions for some of the features:

@leebyron Could you please review this PR and also answer a few questions:

1. It looks likevisit was designed to handle arrays in root as an argument, but this feature is not used anywhere. Is it required functionality?
2. parent of visitor callback is very strange. If the current node is not in an array it has a value of parent node as expected but if it's inside an array then parent is undefined. In this PR I made parent to always point to parent ASTNode as I think it's what's expected. Is it possible to merge such change?
3. key parameter is always equal to the last element of path array and can be either number or string depending on node location. I see that absolute majority of callback ignores this parameter, so maybe it's worth removing it (you always can get the same value from the path) to simplify callback signature?
4. ancestors is an array of both ASTNodes and arrays of ASTNodes which is very confusing + you can get the same array from root using path elements as indexes. In my PR I changed ancestors to be an array of parent ASTNodes. Is it possible to merge such change?

I saw you made a few small breaking changes in the recent PRs. I think it would be great to also include some interface cleanup of visit function. Since at the moment it scares away 3rd-party developers from writing tooling or contributing to existing projects. For example here is an issue reported on GraphQL Faker repository:

We have done some investigation and it appears that the culprit is this method /src/proxy.ts@master#L122. Unfortunately, this is where we run out of depth, as we currently don't have the bandwidth to study the visit method / API properly.

[leebyron]

Thanks for investigating this. I'd prefer if this were broken up into a few smaller PRs since this is a bit difficult to follow. We'll probably need to add more tests against the visitor before refactoring.

Since the issue is understandability of the API - perhaps it's best to start with expanding the documentation above the function and the corresponding type definitions. The implementation should not need to change much if at all.

Later, we can investigate PRs with more focused implementation refactors

[leebyron]

It would be nicer to instead use $PropertyType<T, 'kind'> instead of manually assembling this type

[IvanGoncharov]

My goal was to provide correct types for node argument of callback depending on how it registered, e.g.:

visit(ast, {
  Document(node) {
     // node should be DocumentNode and not ASTNode
  }
});

The only way to do this is to create the type like that:

import type {
  DocumentNode,
  // ...
} from './ast';

type Visitor = {
   enter: VisitorFn<ASTNode> | { 
      Document: VisitorFn<DocumentNode>,
      ...
   },
   Document: VisitorFn<DocumentNode> | { enter?: VisitorFn<DocumentNode>, leave?: VisitorFn<DocumentNode>}
  // ...
}

So I need to list every node type three times. That's why I tried to reduce it to the following:

import type { KindToASTNodeType } from './ast';

type KindVisitor = {|
  ...$ObjMap<KindToASTNodeType, <T>(T) => VisitorFn<T>>,
|};

type Visitor = {|
  enter?: VisitorFn<ASTNode> | KindVisitor,
  leave?: VisitorFn<ASTNode> | KindVisitor,

  ...$ObjMapi<KindToASTNodeType , <T>(T) => NodeVisitor<T>>,
|};

I really like to properly type visitor argument because without it you can achieve type-safety inside visitor callbacks.
I'm new to flow, so can you please give example how I can achieve the same result with $PropertyType<T, 'kind'>?
Or should I just list every node type without trying to use clever tricks?

[leebyron]

It looks like visit was designed to handle arrays in root as an argument, but this feature is not used anywhere. Is it required functionality?

Yes, visit is a generic utility. In the past we've relied on its ability to begin visiting at any point in an AST tree including Arrays. I don't think we currently have this case directly within Graphql.js but I don't see a need to hard-code against it.

parent of visitor callback is very strange. If the current node is not in an array it has a value of parent node as expected but if it's inside an array then parent is undefined. In this PR I made parent to always point to parent ASTNode as I think it's what's expected. Is it possible to merge such change?

Let's focus on changes to the API in separate focused PRs. We have code that relies on this behavior so we would need to include a migration plan - I'm not sure that's something we want to do.

key parameter is always equal to the last element of path array and can be either number or string depending on node location. I see that absolute majority of callback ignores this parameter, so maybe it's worth removing it (you always can get the same value from the path) to simplify callback signature?

There is existing code which relies on this - we could change it in a focused PR with a clear migration plan, but we would need to weigh the benefits with costs. It's not immediately clear to me that it would be worth it.

ancestors is an array of both ASTNodes and arrays of ASTNodes which is very confusing + you can get the same array from root using path elements as indexes. In my PR I changed ancestors to be an array of parent ASTNodes. Is it possible to merge such change?

There are use cases where you need to access sibling items in an array and being able to walk up the ancestors path is used to do so. Again - there may be a subjectively improved API out there, but it comes at the cost of building a migration strategy for existing tools. We should focus on API changes holistically and separate from any refactoring and type/documentation changes

[IvanGoncharov]

Thanks for investigating this. I'd prefer if this were broken up into a few smaller PRs since this is a bit difficult to follow. We'll probably need to add more tests against the visitor before refactoring.

Agree 👍
I will work on PR adding path and ancestors to existing tests.
I'm a little bit worried about size and readability of the test file, e.g. this JSON blog will become huge even if I add the minimum of additional info.
Just as an idea maybe it worth to use snapshot testing (e.g. chai-jest-snapshot) in some of the tests?

Also, I would love to enable Flow in this file and add typings in separate PR. Actually, I tried to do this initially but was overwhelmed by a number of flow errors which is impossible to fix without code changes.
It would be great to enable Flow for the entire file but exclude some tricky parts but the only mechanism I found is $FlowFixMe which only works for a single line and require me to add it in a couple dozens of places making visit function unreadable. Do you how I can disable Flow checks for a block of code?

[mjmahone]

If I were tackling this, I'd probably split this process up. First, I'd create the "common API", well-typed functions. For instance, maybe having one function for calling visit on an AST node with the visitor object only being {[key: NodeKind]: EnterFunction}, which under the hood just delegates to visit. In some ways, this is what we do in relay-compiler/graphql-compiler in the GraphQLIRVisitor.

Once we have well-typed wrapping functions for each specific (and hopefully relatively generic) use case that we think is important, we could then work on migrating existing calls and cleaning up the inner workings of visit in a separate set of breaking-changes.

[leebyron]

I'm new to flow, so can you please give example how I can achieve the same result with $PropertyType<T, 'kind'>?

Unfortunately I couldn't figure it out either. I think your approach of explicitly listing a mapping of kind to node type works pretty well (ideally, I would have flow infer it from the ASTNode union type). I pulled your flow type changes into #1155 which ended up requiring very minimal implementation changes to get both flow and lint to pass cleanly. It helped catch a bug too! Yay!

[odykyi]

create variable key before line 216 and use it in 216 line
const isInArray = typeof key === 'number';
and in 218 line
const curIndex = isInArray ? key : undefined;

[mjmahone]

@IvanGoncharov do you still want to make this refactor? I know there's been a bit of churn in the visitor function, not sure if this is still relevant.

[IvanGoncharov]

Converting into #3225 issue