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

Add ClientOptions to support withCredentials and request debugging #47

Merged
merged 6 commits into from
Sep 25, 2017

Conversation

imalsogreg
Copy link
Owner

Add a new ClientOptions type and clientWithOpts functions.

The options support user's ability to set the withCredentials bit in xhr requests, and to choose a request debugging function that runs in JSM.

Addresses #46 and #45

RFC @3noch and @NorfairKing what do you think?

@NorfairKing
Copy link

I'm happy-ish with this.
I may not know enough about CORS, but don't you ALWAYS want optsWithCredentials = True when using any kind of auth? I'd prefer not to have to think about this myself...

If not, then this is certainly fine.

@3noch
Copy link
Collaborator

3noch commented Sep 25, 2017

@NorfairKing You'd only want withCredentials = True if you're actually doing CORS. If you're using auth on your own domain then you don't need it, which is probably the most common case.

@3noch
Copy link
Collaborator

3noch commented Sep 25, 2017

@imalsogreg Thanks so much for tackling this! I like your approach. I have one more hope for this feature: I'd like to be able to arbitrarily modify the request before it gets sent (for example, setting custom headers). Would it be possible to instead provide some arbitrary Req -> Req function that can do that (including setting withCredentials)? That would solve both of those cases. If the Req -> Req was Req -> JSM Req, then it would tackle all three needs in one swoop.

@NorfairKing
Copy link

@3noch Thanks for the clarification. @imalsogreg Thanks for acting so quickly.

@imalsogreg
Copy link
Owner Author

Latest push removes all the fields from ClientOptions except for optsRequestFixup :: XhrRequest a -> IO (XhrRequest a), as both debug-printing and withCredentials-setting can be implemented in terms of that.

I was considering setting withCredentials automatically based on BaseUrl - True for absolute paths (more likely to be CORS) and False for relative paths (never CORS)... but since the XHR javascript API doesn't do this, I figured it may be best to let library users set it always manually - they likely have a bette grasp on the right & wrong times to use withCredentials than I do :)

I'll merge this pretty soon, unless you have some other preferences?

@3noch
Copy link
Collaborator

3noch commented Sep 25, 2017

@imalsogreg This is great! Thanks for your hard work on this!

@3noch
Copy link
Collaborator

3noch commented Sep 25, 2017

I think you're right not to do anything fancy with withCredentials.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants