-
Notifications
You must be signed in to change notification settings - Fork 470
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
Conversation
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`.
…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
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`.
I think this looks almost right, but the Since The steps to do so are relatively straightforward with the right commands, these which assume that both repositories are using $ 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: 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? 😸 |
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
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. |
@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!) |
52cf9b1
to
bee8eaa
Compare
There was a problem hiding this 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!
…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
…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
…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
…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
This is the first step to moving the new
apollo-graphql
package into this repo, fromapollo-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:
dist
->lib
to match the pattern of this repoesModuleInterop
andtarget: 'es2016'
to match tsconfig of the previous project that it came from@martijnwalraven pointed out the dependency on
node@6
withinapollo-server
, so I left that as is.TODO: