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

Replace axios #1036

Closed
wilsonianb opened this issue Jan 26, 2023 · 5 comments
Closed

Replace axios #1036

wilsonianb opened this issue Jan 26, 2023 · 5 comments
Labels
pkg: auth Changes in the GNAP auth package. pkg: backend Changes in the backend package. pkg: map Changes in mock-account-provider package pkg: token-introspection Changes in the token-introspection package. stale type: enhancement New feature or request

Comments

@wilsonianb
Copy link
Contributor

@wilsonianb wilsonianb added pkg: backend Changes in the backend package. pkg: auth Changes in the GNAP auth package. pkg: map Changes in mock-account-provider package pkg: open-payments pkg: token-introspection Changes in the token-introspection package. labels Jan 26, 2023
@wilsonianb
Copy link
Contributor Author

global fetch is experimental so maybe we hold off?
https://nodejs.org/en/blog/announcements/v18-release-announce/

@sabineschaller sabineschaller added the type: enhancement New feature or request label Mar 29, 2023
@mkurapov
Copy link
Contributor

mkurapov commented Mar 4, 2024

Verify whether nock supports fetch before proceeding

@mkurapov mkurapov added the stale label Mar 5, 2024
@BlairCurrey
Copy link
Contributor

BlairCurrey commented Mar 7, 2024

Just wanted to note a potential gotcha if we switch over to fetch because this took me a while to figure out. The standard lib fetch does not automatically handle the Set-Cookie header as you might expect. We would need to parse this header and use them on subsequent requests ourselves. It may not be clear where exactly we will be receiving such a header so it may make sense to wrap fetch and do this in a central place (else we likely wont catch all cases) if we switch to fetch.

While doing the grant interaction flow (start, accept, finish), I noticed that the "finish" request did not find the koa session started by the initial "start" request. Digging a bit further, this is because the client did not send the session detail cookie on the finish request. The response from the start request contains these details in the Set-Cookie header but fetch doesn't do anything with it, regardless of the credentials option which usually controls it (https://developer.mozilla.org/en-US/docs/Web/API/fetch#credentials).

Thus when using fetch you need to parse the cookies from the Set-Cookie header and provide them on subsequent requests: cc93982#diff-87048db525e9563b074c43d7daba7be936621984eedcf27d2dd78c3fc1e25890R250-R267

edit: TIL this is simply how most http clients work outside the browser 😑 Was getting caught up by our axios client handling cookies in our consent screen but didn't stop to think that the difference is the browser not the client. Then realized axios didn't do this either. I see there are plugins like https://github.com/3846masa/axios-cookiejar-support that add this kind of support but I wonder why its not a more common feature. Perhaps because outside the browser you cant assume non-multi-tenancy?

@mkurapov
Copy link
Contributor

If we end up working on this, I recommend using ky, since it would allow us to easily define a global http client with preset headers (with interceptors/hooks if we choose).

This was done in the open-payments repo already:

@mkurapov
Copy link
Contributor

mkurapov commented Oct 1, 2024

Closing - any new code changes will use native fetch (or ky, as mentioned above)

@mkurapov mkurapov closed this as completed Oct 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: auth Changes in the GNAP auth package. pkg: backend Changes in the backend package. pkg: map Changes in mock-account-provider package pkg: token-introspection Changes in the token-introspection package. stale type: enhancement New feature or request
Projects
Status: Done
Development

No branches or pull requests

4 participants