-
Notifications
You must be signed in to change notification settings - Fork 734
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
Make 'credentials' configurable for GQL requests #470
Conversation
@timsuchanek @HuVik Is the |
It was broken before, i think here 300f250 |
Did you have little time to check this out, @timsuchanek? |
It was already broken, but I'm fixing it right now. Thanks for the PR! |
Thank you for merging it, Tim! |
It appears that the parameters passed via |
I think this should be set to |
@vincenzo you legend this is dope. |
Make 'credentials' configurable for GQL requests
This PR fixes #364
The cause of the issue
The issue was due to this. Tim must've commented that out because having
credentials: "include"
by default is not best practice, as it breaks requests to any server that does not have the correctcors
options set up. Specifically, a server must know the domains of all the requesters in advance whencredentials: "include"
.The proposed change
The change proposed here is very simple:
request.credentials: "omit"
to the default settingsfetch()
call used to "post" GraphQL queries/mutations to the serverBy having
"omit"
as a default setting, I preserve the current behaviour: no request will be broken due to lack of the correctcors
options server-side.Additionally,
request.credentials
can take all the values thatcredentials
forfetch()
takes:"omit"
"include"
"same-origin"
Of course, one must know what they are doing, as the server must have
cors
enabled and correctly configured. Thesame-origin
value comes in handy for those project like the one I am working on right now, where we use an express middleware, so client and server are on the same domain. Also, our official "client", when ready, will also be living on the same domain. Theinclude
value can be set for all other scenarios where the client lives on a different domain than the server and one can tell the server what "client domains" to allow.Tested
I have tested this locally pointing the react app to my server (correctly configured for each of the
credentials
values) and all seems to be working just fine.Potential improvements
It might be that
Settings
is not exactly the best place to put this setting in, but I do believe that it is a good enough choice for the time being.Expose the settings to the middlewares so that they can instantiate the Playground with the option for "credentials" that it is best for their project.