Refactor `visit` function
- 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
Assessment
This issue has not been assessed yet.