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

fix: platformSelf to send cookies and append f=json in url #778

Merged
merged 1 commit into from Nov 13, 2020

Conversation

dbouwman
Copy link
Member

Update the recently added platformSelf function to send cookies as well as ?f=json in the url.

The latter should be a short-term fix as the API should not require it, but currently it will 405 on the OPTIONS pre-flight if that's no in the url as shown

image

@codecov
Copy link

codecov bot commented Nov 13, 2020

Codecov Report

Merging #778 (da5a207) into master (2d36c52) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #778   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          120       120           
  Lines         1915      1915           
  Branches       325       325           
=========================================
  Hits          1915      1915           
Impacted Files Coverage Δ
packages/arcgis-rest-auth/src/app-tokens.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2d36c52...ed81919. Read the comment docs.

@dbouwman dbouwman merged commit 7cc8b02 into master Nov 13, 2020
@noahmulfinger
Copy link
Contributor

@dbouwman @tomwayson Is this setup working for you? request doesn't pass through the credentials property as far as I can tell.

@tomwayson tomwayson deleted the b/add-missing-param-platform-self branch November 17, 2020 15:45
@tomwayson
Copy link
Member

good catch @noahmulfinger. I didn't try it, myself.

Looks like he's right @dbouwman:

@dbouwman
Copy link
Member Author

Thanks @noahmulfinger - the issue is that it's pretty difficult to setup to actually test this fn. I was able to verify that the api works using fetch devtools in a tab on a page where the esri_aopc cookie is set, but I have not created an actual test app using rest-js yet. I'll have to puzzle this a bit... I think we can use the credential value if passed... but I need to verify with the API team how/if/?? should happen in IWA scenarios with this particular function.

Comment on lines 78 to 81
headers: {
"X-Esri-Auth-Client-Id": clientId,
"X-Esri-Auth-Redirect-Uri": redirectUri,
},
Copy link
Member

@tomwayson tomwayson Nov 18, 2020

Choose a reason for hiding this comment

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

@dbouwman @noahmulfinger I wonder if we can use the existence of headers that start w/ "X-Esri-Auth-" as the signal to request() that it should set credentials: "include"?

Rather than introduce any new properties on IRequestOptions.

Copy link
Contributor

Choose a reason for hiding this comment

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

That seems reasonable. Another option would be adding a fetchOptions param or something similar. That way we could allow extra overrides for any future edge cases like this.

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.

None yet

3 participants