Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modernize apollo-utilities graphql transformation functionality #4233

Merged
merged 28 commits into from
Dec 20, 2018

Conversation

hwillson
Copy link
Member

The graphql AST transformation capabilities provided by apollo-utilities are no longer in synch with graphql 14.x, as they rely on being able to directly modify a graphql based AST. graphql 14.x has made the internal document structure read only, so we'll need to update apollo-utilities to use a different approach (likely using graphql's visitor functionality). We've know about this for a while (Renovate keeps reminding us - e.g. #3875), but we should definitely address this sooner than later.

As a start, this PR updates the @types/graphql and graphql dev deps to the latest 14.x based versions, which will break CI. This PR will serve as a placeholder until this is addressed.

This will help avoid typescript errors when using `graphql` 14.x.
Required by `graphql` 14.x, unless of course we want to parse the
incoming read only array, and convert it into a `GraphQLError`
array. If keeping things aligned with `graphql` (by using a
read only error array) proves to be problematic, especially with
regards to backwards compatibility, we might have to change this.
We're using a few es7 level functions, like
`Array.prototype.includes`.
`graphql` 14.x has made most parts of the AST readonly, which means
we can't upgrade Apollo Client to use `graphql` 14.x, until the
`apollo-utilities` transformation helpers are modified to stop
writing back into the original AST. This commit replaces all
cloning / re-writing with a node visitor approach, provided by
`graphql`'s `visit` function.
@hwillson hwillson changed the title [WIP] Modernize apollo-utilities graphql transformation functionality Modernize apollo-utilities graphql transformation functionality Dec 17, 2018
Copy link
Member

@benjamn benjamn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm loving how much code this is going to save!

packages/apollo-utilities/src/transform.ts Outdated Show resolved Hide resolved

let modifiedDoc = visit(doc, {
Variable: {
enter(node, _, parent) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's provide types for these parameters whenever possible, since otherwise they'll just be any.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left types out of the visitor function declarations, as Typescript appears to be able to infer them properly from @types/graphql. If I stored the visitor function object outside of the visit call, then it wouldn't be able to, but since it's part of the visit call it seems okay. Happy to change if desired though!

packages/apollo-utilities/src/transform.ts Outdated Show resolved Hide resolved
packages/apollo-utilities/src/transform.ts Outdated Show resolved Hide resolved
fragmentSpreadsToRemove = fragmentSpreadsToRemove.concat(
fragSpreads.map(frag => ({
name: frag.name.value,
})),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as above:

getAllFragmentSpreadsFromSelectionSet(
  node.selectionSet,
).forEach(frag => {
  fragmentSpreadsToRemove.push(frag.name.value);
});

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, for both fragmentSpreadsToRemove and variablesToRemove, it seems like it's really a set of fragments/variables that might be able to be removed, since they might still be used elsewhere (which is something we check later). Not sure if a renaming is warranted, but thought I would mention.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I couldn't think of better naming; the reason being that fragmentSpreadsToRemove and variablesToRemove are eventually passed into removeFragmentSpreadFromDocument / removeArgumentsFromDocument as a param, and in that case they really do represent xToRemove.

packages/apollo-utilities/src/transform.ts Outdated Show resolved Hide resolved
packages/apollo-utilities/src/transform.ts Outdated Show resolved Hide resolved
packages/apollo-utilities/src/transform.ts Outdated Show resolved Hide resolved
packages/apollo-utilities/src/transform.ts Outdated Show resolved Hide resolved
packages/apollo-utilities/tsconfig.json Outdated Show resolved Hide resolved
@hwillson hwillson merged commit c04600b into master Dec 20, 2018
@hwillson hwillson deleted the hwillson/graphql-updates branch December 20, 2018 19:20
@hwillson hwillson moved this from In Progress to Done in Apollo Client Jan 2, 2019
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
No open projects
Apollo Client
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants