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-graphql package into the apollo-tooling repo #1018

Merged
merged 14 commits into from
Feb 15, 2019

Conversation

trevor-scheer
Copy link
Member

@trevor-scheer trevor-scheer commented Feb 13, 2019

This is the first step to moving the new apollo-graphql package into this repo, from apollo-server.
The second step will be to PR the removal from apollo-server.

This is almost a wholesale copy/paste. A couple changes worth noting:

  • Rename dist -> lib to match the pattern of this repo
  • esModuleInterop and target: 'es2016' to match tsconfig of the previous project that it came from

@martijnwalraven pointed out the dependency on node@6 within apollo-server, so I left that as is.

TODO:

  • Update CHANGELOG.md* with your change (include reference to issue & this PR)
  • Make sure all of the significant new logic is covered by tests
  • Rebase your changes on master so that they can be merged easily
  • Make sure all tests and linter rules pass

As of this commit, this package provides nothing!
 - apollo-graphql@0.0.1-alpha.0
…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`.
…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
 - apollo-cache-control@0.5.0-alpha.3
 - apollo-datasource-rest@0.3.0-alpha.3
 - apollo-engine-reporting@1.0.0-alpha.3
 - apollo-graphql@0.0.1-alpha.1
 - apollo-server-azure-functions@2.4.0-alpha.4
 - apollo-server-cloud-functions@2.4.0-alpha.4
 - apollo-server-cloudflare@2.4.0-alpha.4
 - apollo-server-core@2.4.0-alpha.4
 - apollo-server-express@2.4.0-alpha.4
 - apollo-server-hapi@2.4.0-alpha.4
 - apollo-server-integration-testsuite@2.4.0-alpha.4
 - apollo-server-koa@2.4.0-alpha.4
 - apollo-server-lambda@2.4.0-alpha.4
 - apollo-server-micro@2.4.0-alpha.4
 - apollo-server-plugin-base@0.3.0-alpha.4
 - apollo-server-testing@2.4.0-alpha.4
 - apollo-server@2.4.0-alpha.4
 - apollo-tracing@0.5.0-alpha.3
 - graphql-extensions@0.5.0-alpha.4
 - apollo-cache-control@0.5.0
 - apollo-datasource-rest@0.3.0
 - apollo-datasource@0.3.0
 - apollo-engine-reporting@1.0.0
 - apollo-graphql@0.1.0
 - apollo-server-azure-functions@2.4.0
 - apollo-server-cache-memcached@0.3.0
 - apollo-server-cache-redis@0.3.0
 - apollo-server-caching@0.3.0
 - apollo-server-cloud-functions@2.4.0
 - apollo-server-cloudflare@2.4.0
 - apollo-server-core@2.4.0
 - apollo-server-express@2.4.0
 - apollo-server-hapi@2.4.0
 - apollo-server-integration-testsuite@2.4.0
 - apollo-server-koa@2.4.0
 - apollo-server-lambda@2.4.0
 - apollo-server-micro@2.4.0
 - apollo-server-plugin-base@0.3.0
 - apollo-server-testing@2.4.0
 - apollo-server@2.4.0
 - apollo-tracing@0.5.0
 - graphql-extensions@0.5.0
abernix pushed a commit that referenced this pull request Feb 14, 2019
I observed that the tests weren't running for the `apollo-graphql` package
when reviewing the work in #1018.

Upon investigating further (via `jest --debug`), it seems that the Jest
configuration was missing the appropriate configuration for it to pickup the
extensions of `ts` and `tsx` within the `apollo-graphql` repository.

It was only then that I realized that this repository still has `jest`
configuration defined in each of its `package.json` files, rather than a
monorepo-root `jest.config.base.json` file.  That's fine, but it caught me
off guard.

I do think that we should try to DRY-up this repetition and align this
across teams and repositories as soon as possible because it's difficult to
start building per-package exceptions which will quickly become hard to
manage.  For now, this will get tests working for `apollo-graphql` within
the scope of the PR that brings it into the `apollo-tooling` repository,
which seems necessary to tick the box for shipping that PR.
I observed that the tests weren't running for the `apollo-graphql` package
when reviewing the work in #1018.

Upon investigating further (via `jest --debug`), it seems that the Jest
configuration was missing the appropriate configuration for it to pickup the
extensions of `ts` and `tsx` within the `apollo-graphql` repository.

It was only then that I realized that this repository still has `jest`
configuration defined in each of its `package.json` files, rather than a
monorepo-root `jest.config.base.json` file.  That's fine, but it caught me
off guard.

I do think that we should try to DRY-up this repetition and align this
across teams and repositories as soon as possible because it's difficult to
start building per-package exceptions which will quickly become hard to
manage.  For now, this will get tests working for `apollo-graphql` within
the scope of the PR that brings it into the `apollo-tooling` repository,
which seems necessary to tick the box for shipping that PR.
Tests for `apollo-graphql` are working again as of this commit, but the
tests highlighted interop failures with regards to `esModuleInterop` since
the testing configuration for this repository seems to be partially
configured to use different configurations.  We should probably fix that!

Since this setting isn't inherited from a monorepo root configuration, this
needs to be defined in both the package's `tsconfig.json` and its
`tsconfig.test.json`.
@abernix
Copy link
Member

abernix commented Feb 14, 2019

I think this looks almost right, but the apollo-graphql tests aren't running. I fixed that without too much effort (more on that in a moment) but I also thought I'd comment on the technique for the move itself and try to avoid the wholesale copy/paste which, I think, makes it more difficult to validate the gap between the two repositories (i.e. did anything go wrong?)

Since apollo-graphql is a relatively new repository, it was tempting to not preserve its existing Git history, but I think it's a good habit for us to save that information when possible, rather than have you take the forever-git blame for anything which I may have done wrong with the initial implementation of apollo-graphql which this imports! 😉

The steps to do so are relatively straightforward with the right commands, these which assume that both repositories are using packages/* for their members, and might be worth showing here for posterity:

$ cp -R apollo-server apollo-server-BACKUP
$ cd apollo-server-BACKUP
$ git checkout -b just-apollo-graphql master
$ git filter-branch -f --prune-empty --subdirectory-filter packages/apollo-graphql -- --all
$ git clean -dfx # delete things which are leftover.
$ git filter-branch -f --index-filter '
    git ls-files -sz |
    perl -0pe "s{\t}{\tpackages/apollo-graphql/}" |
    GIT_INDEX_FILE=$GIT_INDEX_FILE.new \
        git update-index --clear -z --index-info &&
        mv "$GIT_INDEX_FILE.new" "$GIT_INDEX_FILE"
' HEAD
$ cd ../apollo-tooling
$ git checkout -b abernix/add-apollo-graphql
$ git remote add local ../apollo-server-BACKUP
$ git pull --allow-unrelated-histories local just-apollo-graphql --no-ff
$ # Complete the merge by solving conflicts and merging them! 🎉

Happy to help you grok these if it's interesting to you, but I went ahead and ran them and got things setup on a new branch in this repository: abernix/add-apollo-graphql and then rebased your changes from this PR on top of it.

The great thing about this technique is that it makes the subtle changes you needed to make very clear, as seen here: 3decc83.

Anyhow, as the branch link above shows, I added two other commits to make testing work:

I think with those last couple commits then this PR is LGTM.

My long-winded proposition is, if this all looks good to you, would you entertain the idea of me force pushing that branch to this PR? 😸

@abernix
Copy link
Member

abernix commented Feb 14, 2019

Refs: apollographql/apollo-server#2316

abernix pushed a commit to apollographql/apollo-server that referenced this pull request Feb 14, 2019
The purpose of this PR is to remove the `apollo-graphql` package from this repository, to be added to `apollo-tooling` instead.

https://github.com/apollographql/apollo-tooling

Ref: apollographql/apollo-tooling#1018
@abernix
Copy link
Member

abernix commented Feb 14, 2019

Sorry, I have no idea why I referred to the above as relatively straightforward. It's obviously not. It was relatively straightforward only because I happened to know it was possible, took very little time to execute, and knew exactly what to reference.

@trevor-scheer
Copy link
Member Author

@abernix thank you as always for the excellent information. Please force push away! We'll shoot for a merge and publish tomorrow (my morning) if you're available. Would love to discuss your technique as well (it's super interesting to me!)

@abernix abernix force-pushed the trevor/add-apollo-graphql branch from 52cf9b1 to bee8eaa Compare February 15, 2019 08:51
Copy link
Member

@abernix abernix left a comment

Choose a reason for hiding this comment

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

This is ready, as far as I'm concerned!

@trevor-scheer trevor-scheer merged commit 414b5fc into master Feb 15, 2019
@trevor-scheer trevor-scheer deleted the trevor/add-apollo-graphql branch February 15, 2019 14:48
trevor-scheer pushed a commit to apollographql/apollo-server that referenced this pull request May 6, 2020
…ist`. (#23)

Since we've moved the `apollo-graphql` package from `apollo-server` to
`apollo-tooling`, it's now subjected to the new TypeScript compilation rules
for that repository.

(See: apollographql/apollo-tooling#1018)

Those compilation rules differ slightly and put their output in the `lib`
directory rather than the `dist` directory.  In an ideal world, we'd fix
that at publish time so the actual `outDir` didn't matter (i.e. we could
actually publish from the `dist/` directory, something which `lerna`
actually supports!), but for now, changing this import should be good.

I'd intentionally not done this before (and instead just pinned the version
exactly to a version which I knew was still utilizing the previous `dist/`
compilation `outDir`) as we'd been waiting for the first publish to occur
from that repository, but that publish has now happened and Renovate has
come through and updated it so it's time to make the change more
permanently!

Follows-up: https://github.com/apollographql/apollo-platform-commercial/pull/10
trevor-scheer pushed a commit to apollographql/apollo-server that referenced this pull request May 12, 2020
…ist`. (#23)

Since we've moved the `apollo-graphql` package from `apollo-server` to
`apollo-tooling`, it's now subjected to the new TypeScript compilation rules
for that repository.

(See: apollographql/apollo-tooling#1018)

Those compilation rules differ slightly and put their output in the `lib`
directory rather than the `dist` directory.  In an ideal world, we'd fix
that at publish time so the actual `outDir` didn't matter (i.e. we could
actually publish from the `dist/` directory, something which `lerna`
actually supports!), but for now, changing this import should be good.

I'd intentionally not done this before (and instead just pinned the version
exactly to a version which I knew was still utilizing the previous `dist/`
compilation `outDir`) as we'd been waiting for the first publish to occur
from that repository, but that publish has now happened and Renovate has
come through and updated it so it's time to make the change more
permanently!

Follows-up: https://github.com/apollographql/apollo-platform-commercial/pull/10
trevor-scheer pushed a commit to apollographql/apollo-server that referenced this pull request May 14, 2020
…ist`. (#23)

Since we've moved the `apollo-graphql` package from `apollo-server` to
`apollo-tooling`, it's now subjected to the new TypeScript compilation rules
for that repository.

(See: apollographql/apollo-tooling#1018)

Those compilation rules differ slightly and put their output in the `lib`
directory rather than the `dist` directory.  In an ideal world, we'd fix
that at publish time so the actual `outDir` didn't matter (i.e. we could
actually publish from the `dist/` directory, something which `lerna`
actually supports!), but for now, changing this import should be good.

I'd intentionally not done this before (and instead just pinned the version
exactly to a version which I knew was still utilizing the previous `dist/`
compilation `outDir`) as we'd been waiting for the first publish to occur
from that repository, but that publish has now happened and Renovate has
come through and updated it so it's time to make the change more
permanently!

Follows-up: https://github.com/apollographql/apollo-platform-commercial/pull/10
trevor-scheer pushed a commit to apollographql/apollo-server that referenced this pull request May 14, 2020
…ist`. (#23)

Since we've moved the `apollo-graphql` package from `apollo-server` to
`apollo-tooling`, it's now subjected to the new TypeScript compilation rules
for that repository.

(See: apollographql/apollo-tooling#1018)

Those compilation rules differ slightly and put their output in the `lib`
directory rather than the `dist` directory.  In an ideal world, we'd fix
that at publish time so the actual `outDir` didn't matter (i.e. we could
actually publish from the `dist/` directory, something which `lerna`
actually supports!), but for now, changing this import should be good.

I'd intentionally not done this before (and instead just pinned the version
exactly to a version which I knew was still utilizing the previous `dist/`
compilation `outDir`) as we'd been waiting for the first publish to occur
from that repository, but that publish has now happened and Renovate has
come through and updated it so it's time to make the change more
permanently!

Follows-up: https://github.com/apollographql/apollo-platform-commercial/pull/10
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

Successfully merging this pull request may close these issues.

2 participants