graphql / graphql/graphql-js

Refactor `visit` function

Open
#3,225 4 comments 1 reaction 0 assignees View on GitHub
enhancement
Dominant language
TypeScript
Stars
20.3k
Forks
2.1k
Avg merge
44m
Merged PRs (30d)
6

Description

Extracted from #1145:

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 like](https://github.com/graphql/graphql-js/blob/master/src/language/visitor.js#L171)`visit` 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.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.