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

Allow apollo-boost to support headers and credentials #3098

Merged
merged 7 commits into from
Jun 5, 2018

Conversation

rzane
Copy link
Contributor

@rzane rzane commented Feb 28, 2018

Apollo Boost should allow you to pass in headers and credentials. Currently credentials is hardcoded to same-origin and it's not possible to set request headers.

@apollo-cla
Copy link

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

Copy link
Contributor

@peggyrayzis peggyrayzis left a comment

Choose a reason for hiding this comment

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

Hi @rzane! Thanks for making this PR. Since you can already configure headers and credentials, I'm not going to merge this.

You can set headers and credentials either statically by passing them as fetchOptions or dynamically by setting them on the context of the operation with the request function. Does that make sense?

@peggyrayzis peggyrayzis closed this Mar 2, 2018
@rzane
Copy link
Contributor Author

rzane commented Mar 2, 2018

Hi @peggyrayzis, I'm pretty confident that static headers can't be passed through fetchOptions. The HTTP link appears to ignore them.

I've created a repro of that issue here: https://gist.github.com/rzane/d6691acf7d4a60dd3a31389ced72b6de

To demonstrate the problem, run the following commands:

$ git clone git@gist.github.com:d6691acf7d4a60dd3a31389ced72b6de.git apollo-boost-repro && \
    cd apollo-boost-repro && \
    yarn install && \
    yarn start

@sgrove
Copy link

sgrove commented Mar 7, 2018

We're also running same behavior with apollo-boost specifically and can't quite figure out how to work around it. Setting either custom headers or credentials in fetchOptions seems to be ignored.

@zoelner
Copy link

zoelner commented Mar 20, 2018

The line that writes header settings Apollo Bost L77

@rzane
Copy link
Contributor Author

rzane commented Mar 20, 2018

@peggyrayzis, can we reopen this PR?

@tyscorp
Copy link

tyscorp commented Apr 4, 2018

@peggyrayzis this is still broken and makes apollo-boost completely unusable for a lot (most?) users.

@tzvc
Copy link

tzvc commented Jun 1, 2018

What's the update on this guys ? Static headers still can't be passed through fetchOptions

@hwillson
Copy link
Member

hwillson commented Jun 1, 2018

@theochampion This will be merged shortly. There is still some work to do here with regards to testing, but I'm planning on having it done today (which means it will be in next Tuesday's release).

Copy link
Member

@hwillson hwillson left a comment

Choose a reason for hiding this comment

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

We should be all set here - thanks for kickstarting this PR @rzane!

@hwillson hwillson merged commit 3fe0063 into apollographql:master Jun 5, 2018
@thecodejack
Copy link

@hwillson any idea when this is expected to be in any of release tag?

@hwillson
Copy link
Member

@thecodejack We're releasing a new version later today! 🙂

@rzane rzane deleted the headers-boost branch June 12, 2018 13:55
@rares-lupascu
Copy link

i waited the entire day for the new version ... is it yesterday or still today?

@thecodejack
Copy link

@hwillson i don't see the release..can we expect still the release later today?

@hwillson
Copy link
Member

@wowzaaa @thecodejack Sorry, yesterday 💯% got away from me! apollo-client 2.3.3 is now published. Thanks!

@thecodejack
Copy link

Thanks ..it is helping 👍

@@ -74,7 +76,8 @@ export default class DefaultClient<TCache> extends ApolloClient<TCache> {
const httpLink = new HttpLink({
uri: (config && config.uri) || '/graphql',
fetchOptions: (config && config.fetchOptions) || {},
credentials: 'same-origin',
credentials: (config && config.credentials) || 'same-origin',
headers: (config && config.headers) || {},
Copy link
Contributor

@alexkrolick alexkrolick Jul 20, 2018

Choose a reason for hiding this comment

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

These should probably have been part of fetchOptions, right? That's the way it's documented.

Choose a reason for hiding this comment

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

I have lost a night trying to figure this out. It is simply in the wrong place...
Since the beginning I was inserting it inside fetchOptions.... 😡

Choose a reason for hiding this comment

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

I also had this issue, the documentation should be clear to place credentials in the root and not fetchOptions as if placed in fetchOptions it will be overwritten by the root credentials whicih defaults to 'same-origin'

Choose a reason for hiding this comment

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

Just had the same problem today... =/

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.