Feature: Add X-Stormpath-Agent header#31
Conversation
|
|
||
| request.open(method.toUpperCase(), path, true); | ||
| request.setRequestHeader('Accept', 'application/json'); | ||
| request.setRequestHeader('X-Stormpath-Agent', pkg.name + '/' + pkg.version); |
There was a problem hiding this comment.
Yes, that might be interesting. I'm going to add that too.
|
I've verified that this works as expected in Chrome, Safari, and Firefox 👍 But please see my comment regarding also including the React version. And also, Robert excluded this header for CORS requests from the Angular SDK. Will you save that for when you add CORS support? |
|
@timothyej Thanks! I don't think it's necessary to exclude the header for CORS requests since it only requires the users to add add support for that header. |
|
Yes, but then it also requires them to do that in order to get it to work, which adds more friction since it doesn't provide any value for the user. I guess that was @robertjd's thoughts as well? |
|
@typerandom please remove the header if the request is cross-domain. Otherwise, the user will have to add our custom header to their server-side configuration. We don't want to burden the developer with this. See DOM API usage here. Thanks! |
|
@robertjd Sure, will fix that. |
|
@robertjd This is now fixed. I refactored a few things at the same time. The important line to review is this one: https://github.com/stormpath/stormpath-sdk-react/pull/31/files#diff-12ab8b19aebc183c7b26d5fe0f102c6bR60 |
|
|
||
| // Only set the X-Stormpath-Agent header if we're on the same domain as the requested URI. | ||
| // This because we want to avoid CORS requests that require you to have to whitelist the X-Stormpath-Agent header. | ||
| if (!utils.isSameHost(uri, window.location.href)) { |
There was a problem hiding this comment.
I think you should remove the !, right? Otherwise this will add the header if they're not on the same host.
There was a problem hiding this comment.
That's correct. I've fixed that.
typerandom
force-pushed
the
feature-add-stormpath-agent-header
branch
from
e00920f to
0605c61
Compare
|
@timothyej Please review again :) |
|
I've verified that this works as expected in Chrome, Safari, and Firefox 👍 |
Feature: Add X-Stormpath-Agent header
|
\o/ thanks! |
3 participants
Adds the X-Stormpath-Agent header to all API requests.
How to verify
$ npm link).$ npm link react-stormpath).$ npm start)./logincontains the headerX-Stormpath-Agent: react-stormpath/0.5.0.Fixes #30.