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

[6.8] Remove unused dependency apollo-server-hapi #107038

Merged
merged 1 commit into from
Jul 29, 2021

Conversation

watson
Copy link
Contributor

@watson watson commented Jul 28, 2021

This dependency doesn't seem to be used anywhere in our code-base. It could be that it's somehow magically required in a dynamic fashion - hopefully CI will complain if that's the case.

This dependency was originally added in #24068 - however, even in that PR the apollo-server-hapi module wasn't referenced anywhere in the code-base. So maybe even then it was a mistake?

@weltenwort I hope you don't mind that I'm requesting a review from you since you originally created #24068 - so hopefully you can shed some light on this 😅

@watson watson added release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting v6.8.18 labels Jul 28, 2021
@watson watson requested a review from weltenwort July 28, 2021 15:37
@watson watson self-assigned this Jul 28, 2021
@watson
Copy link
Contributor Author

watson commented Jul 28, 2021

From the failing tests, it seems that we were actually depending on some of the sub-dependencies of apollo-server-hapi to be installed, which now that this module is removed are no longer installed. Normally in Node.js land this isn't recommended, but maybe the Apollo ecosystem does it this way?

Until I know better, I'll try to update this PR by adding direct dependencies to the modules that we do require instead.

We actually depended upon a few of its sub-dependencies.
So this commit instead adds those as direct dependencies.
@watson watson force-pushed the remove-apollo-server-hapi branch from e847fe4 to 59b1447 Compare July 28, 2021 17:37
@weltenwort
Copy link
Member

Took me a while to realize this is the 6.x codebase 🙈 I remember something about a package update that broke the official integration between apollo and hapi. I suspect it was at that point that we stopped using apollo-server-hapi directly and re-implemented a custom integration layer. Your strategy of instead depending on its dependencies directly sounds reasonable to me:

https://github.com/apollographql/apollo-server/blob/b32e89c06008719d2fb45e5e22a767625ea13c0a/packages/apollo-server-hapi/package.json#L27-L30

@watson watson changed the title Remove unused dependency apollo-server-hapi [6.8] Remove unused dependency apollo-server-hapi Jul 28, 2021
@watson
Copy link
Contributor Author

watson commented Jul 28, 2021

@weltenwort Sorry for not stating that clearly 😅 I've updated the title now to include [6.8]

And thanks for looking into this 👍

I opened this PR originally because I wanted to either upgrade or get rid of apollo-server-core. If I can get your 👍 in a review, then I'll merge this as is and open another PR where I then try to upgrade apollo-server-core.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

  • 💔 Build #141395 failed e847fe48611e0c5b76aa5903c9fc3594efc61994

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

Copy link
Member

@weltenwort weltenwort left a comment

Choose a reason for hiding this comment

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

Thank you!

@watson watson merged commit 63f7434 into elastic:6.8 Jul 29, 2021
@watson watson deleted the remove-apollo-server-hapi branch July 29, 2021 07:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes v6.8.18
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants