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

apollo-server-core: Inline defaultUsageReportingSignature (for graphql@16) #5955

Merged
merged 1 commit into from
Dec 17, 2021

Conversation

glasser
Copy link
Member

@glasser glasser commented Dec 17, 2021

In v3.5 we made AS compatible with the changes in graphql@16, but
apollo-server-core had a dependency on the somewhat inactive
apollo-graphql package which doesn't yet allow graphql@16 in its peer deps.

We should eventually publish a new version of apollo-graphql (eg,
apollo-server-plugin-operation-registry also depends on it) but the
Apollo Server usage reporting plugin is the only consumer of
defaultUsageReportingSignature (and it is not important that the
individual transforms that make it up stay compatible with the building
blocks of other functions like the operation registry signature
function) so inlining seems most natural.

Fixes #5941.

…l@16)

In v3.5 we made AS compatible with the changes in graphql@16, but
apollo-server-core had a dependency on the somewhat inactive
apollo-graphql package which doesn't yet allow graphql@16 in its peer deps.

We should eventually publish a new version of apollo-graphql (eg,
apollo-server-plugin-operation-registry also depends on it) but the
Apollo Server usage reporting plugin is the only consumer of
defaultUsageReportingSignature (and it is not important that the
individual transforms that make it up stay compatible with the building
blocks of other functions like the operation registry signature
function) so inlining seems most natural.

Fixes #5941.
@glasser glasser force-pushed the glasser/apollo-graphql-inline branch from ca6646c to 68b5379 Compare December 17, 2021 01:04
@glasser
Copy link
Member Author

glasser commented Dec 17, 2021

I tweaked comments a bit but otherwise the code is identical to the code in apollo-graphql. (Well, I did change the typing on the sorted function so that we didn't have to import ListIteratee from @types/lodash given that we only pass strings in practice.)

Copy link
Member

@trevor-scheer trevor-scheer left a comment

Choose a reason for hiding this comment

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

LGTM!

@glasser glasser merged commit e468367 into main Dec 17, 2021
@glasser glasser deleted the glasser/apollo-graphql-inline branch December 17, 2021 01:17
glasser added a commit that referenced this pull request Dec 21, 2021
In #5955 we inlined defaultUsageReportingSignature into
apollo-server-core from apollo-graphql but left its tests behind. This
brings the tests over too. The tests themselves including snapshots are
verbatim from apollo-graphql.
glasser added a commit that referenced this pull request Dec 21, 2021
In #5955 we inlined defaultUsageReportingSignature into
apollo-server-core from apollo-graphql but left its tests behind. This
brings the tests over too. The tests themselves including snapshots are
verbatim from apollo-graphql.
@glasser
Copy link
Member Author

glasser commented Dec 31, 2021

Whoops, this PR failed to actually change the import to the inlined code!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support graphql@16 by fixing dependency on apollo-graphql
2 participants