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

refactor functions used in apollo-server and the router-bridge into apollo-utils #1841

Closed
o0Ignition0o opened this issue May 10, 2022 · 5 comments
Assignees

Comments

@o0Ignition0o
Copy link
Contributor

o0Ignition0o commented May 10, 2022

During our recent work on the router-bridge we copy/pasted bits of code from apollo-server and refactored a bit as well.

It would be great if we could refactor this and use the same methods into apollo-utils

@o0Ignition0o o0Ignition0o changed the title refactor functions used in apollo-server and the router-bridge into internals refactor functions used in apollo-server and the router-bridge into apollo-utils May 10, 2022
@trevor-scheer
Copy link
Member

@glasser maybe a good argument for making this a separate package, though exported by @apollo/server is effectively the same.

@o0Ignition0o can this not be imported as is right now? (I haven't dug to look more closely yet)

@benweatherman
Copy link
Contributor

Doing some spot-checking, it seems like a lot of the copypasta is exported already (e.g. https://github.com/apollographql/apollo-server/blob/d75c6cf3360a46ebcd944b2113438be8f549ae6f/packages/apollo-server-core/src/plugin/usageReporting/operationDerivedDataCache.ts#L5-L8)

I'm wondering if this question came about because the rooter depending on apollo-server-core isn't a great option? I think currently it's just depending on the query-planner, so bringing in server might be a tougher sell.

@trevor-scheer
Copy link
Member

Just spoke with @glasser about this and we actually already have a precedent for this. We recently moved a very similar signature function (operation registry) to its own package in (@apollo/utils.<xyz>)[https://github.com/apollographql/apollo-utils/tree/main/packages/operationRegistrySignature], doing the same for this function and having it live as a single package makes a lot of sense.

@glasser
Copy link
Member

glasser commented May 10, 2022

yeah i believe the task is roughly:

  • do a quick three-way diff between each of the component functions in router, AS, and utils to make sure they're ~identical
  • add remove aliases and the top level usage reporting signature function to utils
  • update AS and router to use the new package rather than their own copies
  • consider whether this is a reasonable time to put usage reporting: switch to stripIgnoredCharacters apollo-server#6001 into play. i think it might be — instead of it being a sorta backwards incompatible thing for two project it'll only be for one

trevor-scheer added a commit to apollographql/apollo-utils that referenced this issue May 19, 2022
This commit introduces a new `@apollo/utils.usagereporting` package
which exports common bits of usage reporting code shared between
apollo-server and router-bridge.

Consequently, this also introduces and updates a couple other related packages:
`@apollo/utils.removealiases`
`@apollo/utils.stripsensitiveliterals`

These transforms are used by `usagereporting` in order to derive the operation
signature.

For testing purposes, I've also introduced a snapshot serializer for printing
GraphQL AST nodes.
`@apollo/utils.jest-graphql-ast-serializer`

Related: apollographql/federation#1841
@trevor-scheer
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants