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

apollo-server-fastify's context function now receives reply object. #3895

Merged
merged 1 commit into from
Jul 15, 2020
Merged

apollo-server-fastify's context function now receives reply object. #3895

merged 1 commit into from
Jul 15, 2020

Conversation

HW13
Copy link
Contributor

@HW13 HW13 commented Mar 16, 2020 •
edited by abernix
Loading

Problem

The context function in apollo-server-fastify is not receiving the reply object. This is due to the request and reply being spread into graphqlServerOptions(in resolveGraphqlOptions via runHttpQuery) - but graphqlServerOptions is expecting an object (here).

Related Issues

Solution

Add applyContextArgs function which receives the reply and request being spread in, and places them into an object which is then passed into graphqlServerOptions.

Test plan

Confirm unit tests pass:

$ cd packages/apollo-server-fastify && jest

Closes #3156

@apollo-cla
Copy link

@HW13: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

@HW13
Copy link
Contributor Author

HW13 commented Mar 18, 2020

Here's a quick test to confirm the existence of this bug -> https://github.com/autotelic/apollo-fastify-bug-test. Cheers.

@jsumners
Copy link

@HW13 it looks like this will also fix an issue where Apollo is stripping Fastify request decorators. Is that true?

@HW13
Copy link
Contributor Author

HW13 commented Mar 18, 2020

I was able to access request decorators in my tests, but I don't think these changes had an affect on that. This PR just adds a function that nests the request and reply in an object before passing it to graphqlServerOptions, so any issues with the request object itself should still exist.

@jsumners
Copy link

Thanks.

@HW13 HW13 mentioned this pull request Apr 24, 2020
Closed
jmcdo29 added a commit to nestjs/throttler that referenced this pull request Jun 11, 2020
@jmcdo29
feat(graphql): implements graphql limiter
1490651 
The GraphQL throttler works well for Express but it does have problems when it comes to fastify due
to [this pull request](apollographql/apollo-server#3895). Once this is
merged in we can start looking at supporting Fastify as well for GraphQL.
jmcdo29 added a commit to nestjs/throttler that referenced this pull request Jun 12, 2020
@jmcdo29
feat(graphql): implements graphql limiter
9bc26d0 
The GraphQL throttler works well for Express but it does have problems when it comes to fastify due
to [this pull request](apollographql/apollo-server#3895). Once this is
merged in we can start looking at supporting Fastify as well for GraphQL.
jmcdo29 added a commit to nestjs/throttler that referenced this pull request Jun 12, 2020
@jmcdo29
feat(graphql): implements graphql limiter
40eaff1 
The GraphQL throttler works well for Express but it does have problems when it comes to fastify due
to [this pull request](apollographql/apollo-server#3895). Once this is
merged in we can start looking at supporting Fastify as well for GraphQL.
@abernix abernix closed this Jun 24, 2020
@jsumners
Copy link

Has this been closed because the issue has been addressed?

@abernix
Copy link
Member

abernix commented Jun 24, 2020

Unintentional closing! Please stand-by: #4304

@abernix abernix reopened this Jun 25, 2020
@abernix abernix changed the base branch from master to main June 25, 2020 09:02
abernix
abernix requested changes Jun 25, 2020
View reviewed changes
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.

I think my only request here is that we align with the patterns in the rest of the repository (on other integrations) as much as possible. I've replied within below about that.

Otherwise, this PR should also update this API documentation.

packages/apollo-server-fastify/src/ApolloServer.ts Outdated Show resolved Hide resolved
packages/apollo-server-fastify/src/ApolloServer.ts Outdated Show resolved Hide resolved
@abernix abernix self-assigned this Jun 25, 2020
@HW13
Copy link
Contributor Author

HW13 commented Jun 25, 2020 •
edited
Loading

Hey @abernix, thanks for the review! The changes you've requested have been implemented. When you have a chance, could you give this another look? Cheers.

@HW13 HW13 requested a review from abernix June 25, 2020 20:21
@HW13 HW13 force-pushed the master branch 2 times, most recently from 1bbeb2d to d946d1e Compare June 26, 2020 22:38
@HW13
Copy link
Contributor Author

HW13 commented Jun 26, 2020

Quick update -> I needed to make some minor tweaks (createGraphqlServerOptions -> createGraphQLServerOptions). I've also squashed my commits and rebased with main - hence the force pushing. Hope that's all good, cheers. 😎 👍

abernix
abernix approved these changes Jul 15, 2020
View reviewed changes
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.

LGTM! I'll add a CHANGELOG.md and get this merged momentarily. (Likely released later today.)

@abernix abernix merged commit 090586c into apollographql:main Jul 15, 2020
@abernix abernix added this to the Release 2.16.0 milestone Jul 15, 2020
abernix added a commit that referenced this pull request Jul 15, 2020
@abernix
Add CHANGELOG.md for #3895
43470d6
@abernix
Copy link
Member

abernix commented Jul 17, 2020

📣 Published in [email protected]! Thanks so much for putting this together originally! I appreciated your contribution efforts and follow-through.

@Sytten
Copy link

Sytten commented Jul 23, 2020

@HW13 @abernix This is a breaking change and should have been a major bump. Previously we have the req parameter that we relied on that was passed to the context function. It is not true that nothing was passed. Now that the version is out, it is a bit too late to revert and push a major, but this should be documented in the README of the package so that people are aware.

abernix added a commit that referenced this pull request Sep 15, 2020
@abernix
Re-apply lost Fastify context signature changes (from #3895)
3e7d7e0 
It appears the branch that introduced this documentation was forked from
prior to that change and that this particular changedidn't get pulled back
in during conflict resolution.

Original: 090586c#diff-c7c64f07d21dfa37133d976476500b03R65
abernix added a commit that referenced this pull request Sep 15, 2020
@StephenBarlow @abernix
docs: New API reference visual treatment (#4494)
5807c90 
* Content-complete on ApolloServer constructor and EngineReportingOptions updates
* Fix some broken links
* Fix some broken headers
* Incorporate #4426 change and merge segmented tables
* Use HTML-md-hybrid table to enable custom styles
* Use new API reference front matter and update some headers
* Use two-column field table layout
* Intentionally leave old EngineReportingOptions format,
  for more optimal formatting, since it's going away soon anyway.
* Put experimental AS constructor option in its own table
* Re-apply lost Fastify context signature changes (from #3895)
* Re-apply lost spelling correction (from #4473)

Co-authored-by: Stephen Barlow <[email protected]>
Co-authored-by: Jesse Rosenberger <[email protected]>
This was referenced May 29, 2021
Closed
Open
Merged
This was referenced Jun 6, 2021
Open
Open
Open
Open
Open
Open
Closed
Open
Open
This was referenced Jun 23, 2021
Open
Merged
This was referenced Jul 1, 2021
Open
Closed
Merged
This was referenced Jul 8, 2021
Closed
Closed
Merged
Merged
Closed
Closed
Closed
@renovate renovate bot mentioned this pull request Aug 7, 2021
Closed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@abernix abernix abernix approved these changes

Assignees

@abernix abernix

Labels
None yet
Projects
None yet
Milestone
Release 2.16.0
Development

Successfully merging this pull request may close these issues.

Not able to access response object from context in apollo-server-fastify
5 participants
@HW13 @apollo-cla @jsumners @abernix @Sytten