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

Move apollo-engine-reporting signatures into apollo-graphql. #2259

Merged
merged 11 commits into from
Feb 4, 2019

Conversation

abernix
Copy link
Member

@abernix abernix commented Feb 1, 2019

Many of these signature calculation functions are now utilized in tools or helpers which are not directly related to apollo-server functionality, including various aspects of the apollo CLI which live within the apollo-tooling repository.

Currently, because of apollo's dependency on apollo-engine-reporting for
this signature, this requires bringing in the entire dependency tree since apollo-engine-reporting depends on apollo-server-core.

By moving this into this new apollo-graphql utility library, we're able to trim that rather hefty dependency tree and drastically reduce the download for running, say, npx apollo.

Furthermore, this PR makes the additional change of switching to the modular lodash.sortby package instead of using the whole of lodash. (The only place we use lodash in the entire apollo-server
repository is to utilize the sortBy function for this signature generation.)

Looking at the bundle stats, it appears that lodash represents 7.1% of the apollo-server package. We're a server, so bundle size is generally less of a concern, but it's still not to be ignored and it's something we want to work on, particularly as we move into worker environments. But, back to the immediate concern about the apollo CLI, since this package will be utilized by the apollo CLI, we'll be shaving additional precious download time off the invocation of npx apollo if we can get this down.

We'll still depend on @types/lodash for just the ListIteratee type (since the modular packages don't provide the lodash/common types) and we should be able to trim 55.4kB minified (19.1kB minified+gzip'd) off the apollo-server build.

cc @trevor-scheer @jbaxleyiii @martijnwalraven

As of this commit, this package provides nothing!
…graphql`.

Many of these signature calculation functions are now utilized in tools or
helpers which are not directly related to `apollo-server` functionality,
including various aspects of the `apollo` CLI which live within
`apollo-tooling`.

Currently, because of `apollo`'s dependency on `apollo-engine-reporting` for
this signature, this requires bringing in the entire dependency tree which
`apollo-server-core` relies on since `apollo-engine-reporting` depends on
`apollo-server-core`.

By moving this into this new `apollo-graphql` utility library, we're able to
trim that rather hefty dependency tree and drastically reduce the download
for running, say, `npx apollo`.
@glasser
Copy link
Member

glasser commented Feb 1, 2019

Moving this out of the reporting package sounds like a great idea.

A few thoughts:

  • This is a backwards-incompatible change to apollo-engine-reporting due to the lack of exports. I think you understand this (the major version bump, etc) but I'd be curious to know if we had any knowledge of people actually using the exports to pass a custom calculateSignature in.
  • Looks like the documentation of calculateSignature in apollo-server.md could at least a tiny update to reference the new package.
  • What if the new package's name was more specific? apollo-graphql is pretty vague (it's basically our company's name!) How about something more like apollo-graphql-query-format or apollo-graphql-query-normalize? Note that my personal instinct is to try to eventually move our company towards a world where "signature" is literally just a well-defined lossless whitespace and sort order normalization.

…odule.

These AST visitors and transformations are more generally usable for other
purposes rather than just the Apollo Engine signature reporting and would
seem to belong in a module of their own.
Currently, the only place that we use `lodash` in the entire `apollo-server`
repository is to utilize the `sortBy` function in this signature generation.

Looking at the bundle stats, it appears that lodash represents 7.1% of the
`apollo-server` package.  We're a server, so bundle size is generally less
of a concern, but it's still not to be ignored, particularly as we move into
worker environments.  More pressingly though, since this package will be
utilized by the `apollo` CLI, we'll be shaving precious download time off
the invocation of `npx apollo` if we can get this down.

By switching to the modular package (but still depending on `@types/lodash`
for _just_ the `ListIteratee` type — which we only need in development — we
should be able to trim 55.4kB minified (19.1kB minified+gzip'd) off the
`apollo-server` build.

cc @trevor-scheer @jbaxleyiii @martijnwalraven
@abernix abernix force-pushed the abernix/engine-signatures-to-apollo-graphql branch from 7662d53 to e84aca2 Compare February 1, 2019 18:40
@abernix
Copy link
Member Author

abernix commented Feb 4, 2019

Concretely, once published and consumed this solves an undesired dependency on apollo-server-core by the apollo CLI. As it stands right now though, those signature algorithms also need to be used by both the apollo-tooling repository (which houses the apollo CLI) and (this) apollo-server repository (which houses apollo-engine-reporting).

We can explore moving formatting functions into another package if it makes sense but there are more concrete plans (which, admittedly, are yet to be fully publicized and finalized) for this package aside from it being a home for signature algorithms, which is certainly not its sole purpose.

@abernix abernix merged commit 060ec62 into master Feb 4, 2019
@abernix abernix deleted the abernix/engine-signatures-to-apollo-graphql branch February 4, 2019 11:06
@apollographql apollographql deleted a comment from glasser Mar 2, 2019
trevor-scheer pushed a commit that referenced this pull request May 6, 2020
As of #2259, the signature
normalization transformations no longer live in `apollo-engine-reporting`.

They now live in `apollo-graphql` which allows us to drop a large portion of
the dependency tree since `apollo-engine-reporting` currently depends on
`apollo-server-core` for its type definitions.

We'll use path-based imports at the moment because not everything is
exported from the main module of `apollo-graphql` (intentionally).
trevor-scheer pushed a commit that referenced this pull request May 12, 2020
As of #2259, the signature
normalization transformations no longer live in `apollo-engine-reporting`.

They now live in `apollo-graphql` which allows us to drop a large portion of
the dependency tree since `apollo-engine-reporting` currently depends on
`apollo-server-core` for its type definitions.

We'll use path-based imports at the moment because not everything is
exported from the main module of `apollo-graphql` (intentionally).
trevor-scheer pushed a commit that referenced this pull request May 14, 2020
As of #2259, the signature
normalization transformations no longer live in `apollo-engine-reporting`.

They now live in `apollo-graphql` which allows us to drop a large portion of
the dependency tree since `apollo-engine-reporting` currently depends on
`apollo-server-core` for its type definitions.

We'll use path-based imports at the moment because not everything is
exported from the main module of `apollo-graphql` (intentionally).
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 22, 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.

2 participants