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.

Sign up for GitHub

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

Jump to bottom

Expose composed middleware via getMiddleware(). #2435

Merged
merged 28 commits into from
Jun 30, 2019
Merged

Expose composed middleware via getMiddleware(). #2435

merged 28 commits into from
Jun 30, 2019

Conversation

ganemone
Copy link
Contributor

@ganemone ganemone commented Mar 12, 2019 •
edited
Loading

This PR exposes the composed koa middleware via the server.getMiddleware() function. This is useful for integrating with systems which may not expose the koa app directly such as Fusion.js. This is an alternative approach to try and resolve some comments on #2244

  • 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

@KevinGrandon KevinGrandon mentioned this pull request Mar 12, 2019
Closed
4 tasks
@ganemone
Copy link
Contributor Author

Anyone have a sec for a review here? This would unblock us from integrating with fusion.js. @martijnwalraven / @benjamn

@ganemone
Copy link
Contributor Author

ping @abernix - any comments?

@abernix abernix self-assigned this Apr 8, 2019
@abernix abernix added this to the Release 2.5.0 milestone Apr 8, 2019
@abernix abernix modified the milestones: Release 2.5.0, Release 2.6.0 Apr 30, 2019
@abernix
Copy link
Member

abernix commented Jun 28, 2019

@ganemone Thank you so much for taking the time to put this together — I really respect this alternate approach and plan to get it in.

That said, we need to try to main some API consistency across (at least our most popular) integrations), with Express being the most pressing.

To maintain that consistency, I took the time to apply the same concept you've used here to apollo-server-express. It seems to work well and I'll be trying to finish that PR that soon, but am a bit limited on time. If anyone was interested in picking it up and testing it, that could help expedite this and allow us to land with a consistent API. It also contains an attempt at Hapi, but can't say that's polished yet.
Anyhow, my PR is built on top of this PR (and makes a couple changes that I'd have requested), but if anyone is interested in taking a peak, here it is: https://github.com/apollographql/apollo-server/compare/abernix/pr-2435

To be clear, I think Express, Hapi and Koa are the main candidates here. Other integrations which already have a "handler" are already a bit more "exposed".

Lastly, I'm tempted to call this exposed method getComposedMiddleware or along the lines of getHighOctaneMiddleware, rather than just getMiddleware, but curious if anyone has any thoughts here. Yes, currently there is applyMiddleware, but I do see a future where there could be a getBareMiddleware (getting the naming and API right here is really my concern) or applyMiddleware could apply something more minimal than what it currently does (See also: #2360).

@abernix abernix modified the milestones: Release 2.6.0, Release 2.x Jun 28, 2019
@ganemone
Copy link
Contributor Author

@abernix - I included your changes into this PR. I also looked into hapi but I don't think this pattern makes much sense for hapi because of how hapi has multiple middleware stacks. Express and koa both have a single middleware stack so exposing a composed middleware makes a lot of sense. I understand the desire for API consistency, and this gets us closer to that, but I also think that things will never be 100% identical simply due to the fundamental differences between some of these underlying server abstractions.

@abernix abernix modified the milestones: Release 2.x, Release 2.7.0 Jun 29, 2019
@abernix abernix changed the title Expose koa middleware via server.getMiddleware() function Expose composed middleware via getMiddleware(). Jun 30, 2019
@abernix abernix changed the base branch from master to release-2.7.0 June 30, 2019 13:48
@abernix abernix merged commit 4149a97 into apollographql:release-2.7.0 Jun 30, 2019
@abernix abernix mentioned this pull request Jun 30, 2019
Closed
4 tasks
@pechitook
Copy link
Contributor

Thank you for this! 🎉

This was referenced Jul 2, 2019
Closed
Closed
Closed
Closed
abernix added a commit that referenced this pull request Jul 16, 2019
@abernix
Revert "Expose composed middleware via getMiddleware()"
d05aceb 
This reverts commit 4149a97 from #2435,
which has resulted in a change to middleware endpoints which could be
perceived as major and breaking.
@abernix abernix mentioned this pull request Jul 16, 2019
Merged
abernix added a commit that referenced this pull request Jul 16, 2019
@abernix
Revert "Expose composed middleware via getMiddleware()" (#3046)
52ab22e 
This reverts commit 4149a97 from #2435,
which has resulted in a change to middleware endpoints which could be
perceived as major and breaking.
abernix added a commit that referenced this pull request Jul 16, 2019
@abernix
Re-land "Expose composed middleware via getMiddleware()"
d24be2d 
This reverts commit d05aceb which was
originally #2435, but was
reverted in 52ab22e (#3046) as a
precaution, prior to releasing 2.7.0.  This attempts to reland it.
@abernix abernix mentioned this pull request Jul 16, 2019
Merged
abernix added a commit that referenced this pull request Aug 23, 2019
@abernix
Re-land "Expose composed middleware via getMiddleware()" (#3047)
6d9c3b8 
* Re-land "Expose composed middleware via getMiddleware()"

This reverts commit d05aceb which was
originally #2435, but was
reverted in 52ab22e (#3046) as a
precaution, prior to releasing 2.7.0.  This attempts to reland it.

* Use an `express.Router` rather than `compose-middleware`.

Aside from avoiding unnecessary dependencies, by using this Router, we avoid
the `apollo-server` base package from behaving different than it has in the
past.  Specifically, `apollo-server` uses `/` as the default path, but it
does so in a wild-card way which serves GraphQL requests on _any_ path.
That means that `/graphqlllll` and `/graphql` both also served the GraphQL
Playground interface, and also responded to GraphQL execution requests.

Since some may be leveraging that behavior, we should preserve it, if we
can.

* Add CHANGELOG.md link to #3184, for awareness.
@abernix
Copy link
Member

abernix commented Aug 23, 2019

Finally landed this in Apollo Server 2.9.0, with some adjustments. 😄 🎉

Copying and pasting my comments from #1308 (comment), since the proposal I mentioned is certainly a direction I see us going, and this seems like a good audience to float it by:

Ok, after recovering from the need to revert it, the getMiddleware bit that I suggested (just) above in #1308 (comment) finally landed in Apollo Server 2.9.0 (thanks to #3047). (Finally!)

That said, I've made a proposal for what I hope is an even better and more flexible/customizable solution in the future. Those subscribers to this issue seem like a relevant audience to solicit for input, so please do take a look at the proposal I put together in #3184! 😄

(For now, I hope getMiddleware will help y'all.)

@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.
Reviewers

@leonaves leonaves leonaves approved these changes

@KevinGrandon KevinGrandon KevinGrandon approved these changes

@abernix abernix Awaiting requested review from abernix

@martijnwalraven martijnwalraven Awaiting requested review from martijnwalraven

Assignees

@abernix abernix

Labels
None yet
Projects
None yet
Milestone
Release 2.7.0
Development

Successfully merging this pull request may close these issues.
5 participants
@ganemone @abernix @pechitook @KevinGrandon @leonaves