Bring visitorKeys back

[ardatan]

See #3245 (comment)

I checked Allow edits by maintainers. So you can make changes on this PR instead of creating a new one by checking out this branch using GitHub CLI. Also you can leave comments here then I can also do the changes you want :)

[linux-foundation-easycla]

The committers are authorized under a signed CLA.

• ✅ Arda TANRIKULU (8b41d52)

[IvanGoncharov]

@ardatan Need to think about this one.
The original intention was was to allow you to use non-standard AST.
Can you please provide examples of code where you use it?

[IvanGoncharov]

Note: you can return false from visitors to skip the traversal of subtrees and it works in old version of graphql-js too.

[ardatan]

Here some examples;
https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby-source-graphql/src/batching/merge-queries.js#L170
https://github.com/ardatan/graphql-tools/blob/master/packages/delegate/src/finalizeGatewayRequest.ts#L285
https://github.com/ardatan/graphql-tools/blob/master/packages/delegate/src/prepareGatewayDocument.ts#L475

Not sure if returning false is the same with visitorKeys because in visitorKeys we can define which parts of each node kind we want to visit.

cc @yaacovCR

[yaacovCR]

Returning false means you no longer visit any of the children, what if you want to visit only some?

[yaacovCR]

@IvanGoncharov why exactly was it deprecated even in terms of its original rationale of custom ast?

in general, for use cases above, it lets you whitelist what you want to visit rather than entering each child node with a visitor function that returns false, the latter being slightly less performant, and sometimes quite more verbose.

[yaacovCR]

Related: ardatan/graphql-tools#3481

[yaacovCR]

we can workaround the above bug secondary to lack of visitorKeys, but it doesn't change that we are doing too much visiting :(

we want to be speedy; if visitorKeys is dropped upstream, we will have to re-implement the v15 version -- please keep

[IvanGoncharov]

I checked Allow edits by maintainers. So you can make changes on this PR instead of creating a new one by checking out this branch using GitHub CLI. Also you can leave comments here then I can also do the changes you want :)

@ardatan That's great, however, I created new PR for a different reason.
In this and your previous PRs, you add code that doesn't match style of the whole file.
A test case that you added in this PR gives a good example of what I'm talking about.
All other test cases follow the same patterns and the same name convention so I expect one that you added to also do this.
I'm 100% up for providing necessary context when reviewing PR but I expect the PR authors to review files they are changing and code around the change.
Without that, I need to check every line of the proposed change and match it with the rest of the code (very hard in GitHub review UI) and spend time left review comments.
And that is a pretty time-consuming and exhausting process, so it's simpler for me to write code from scratch and create new PR.
For example, even though this PR is added single test case it took me way more time to review it and leave comments than to simply from it from scratch using other test cases as templates.

This situation repeats itself and was the major issue with all your previous PRs.
So if you want your future PRs to be merged please spend time and try to match the code style and conventions used in this project. If you to improve something (e.g. typings) do it in separate PR and for all places that have the pattern, you want to change.

P.S. I applied changes to this PR and merging it, but the above comment stays true for future PRs.

[IvanGoncharov]

@yaacovCR I'm merging this one but I think it's not a good long-term solution to expose such a low-level mechanism and also think it's brittle and non-intuitive API. So it should be a part of #3225 and want to continue the discussion there.

[IvanGoncharov]

@ardatan Added ASTVisitorKeyMap to v15 see #3267 will release new version of v15 shortly

[leszekhanusz]

Another use for the visitorKeys is present in graphql-python/gql to parse the results coming from the backend