-
-
Notifications
You must be signed in to change notification settings - Fork 616
WIP: Fetch support #2166
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
WIP: Fetch support #2166
Conversation
So I spent a few hours hacking away on this and it seems to be a heavier lift than the initial work that was done in #797, which was before this library's move to TypeScript. There is quite a bit of logic that follows the callback pattern from I think we should really move away from |
@@ -55,11 +55,11 @@ | |||
"browser-request": "^0.3.3", | |||
"bs58": "^4.0.1", | |||
"content-type": "^1.0.4", | |||
"cross-fetch": "^3.1.5", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be worth investigating but this package uses whatwg-fetch (browser polyfill) which may create downstream issues in Element Web inflating bundle size with a polyfill which we never care about as all compatible browsers already support fetch.
*/ | ||
request?: IHttpOpts["request"]; | ||
fetch?: typeof fetch; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think maybe instead of depending on cross-fetch we could just have the caller pass their favourite fetch, as for typing a shim in /@types/
would be fine
@@ -35,6 +34,7 @@ import { IDeferred } from "./utils"; | |||
import { Callback } from "./client"; | |||
import * as utils from "./utils"; | |||
import { logger } from './logger'; | |||
const queryString = require('qs'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imports not requires please
@t3chguy thank you very much for the look. As I hope was clear, this is very much POC/WIP and I do not have any working code leveraging this yet. As explained in some of the linked issues, my personal motivation here is to be able to support refresh tokens via a Anyway, I'm curious your thoughts on what this change would mean re: backwards compatibility, particularly as it seems all of the callers to the HTTP client's public methods should change away from the For the moment I have backed out of this rabbit trail for my project and am working on shimming in fetch as a custom function that can be used with the same signature... we'll see how that works out! |
I think it won't be that simple, I think @turt2live is also working on refresh tokens, but the issue is if you have multiple tabs open then they will compete with each other and all battle for refreshing so you need a more resilient way to make sure it only gets done by a single tab. |
refresh tokens are sufficiently complicated where they cannot be handled by fetch middleware. The concern isn't that they'll overwrite each other (ultimately it's not bad if they do) - it's that both the old and new token have a strong likelihood of being used, which will trigger the server to do a proper logout. The code on my end already exists. It's just undergoing polish and cleanup before going up for review. |
Thanks for the feedback.
In my case I'm working so far in a React Native context, so I don't have to worry about coordinating multiple tabs... but I will have a web version and this is indeed an issue, there. Thanks for helping me better understand the trade-offs. @turt2live looking forward to seeing your work on refresh tokens. Beyond this question of course is changing away from |
@turt2live, did your work on refresh tokens ever land anywhere? My shim is kinda-sorta doing the trick in React Native but I'd like to move along to something less janky if it will land in the core SDK. Let me know how I can help. |
Ah, ok... I noted the related commits at #2141 (comment) - looks like functions to help perform refresh were added in the SDK and then the implementation was added then reverted in the react implementation. I'm going to close this since my code is very half-baked and I'm not actively working on this. Switching to |
This PR is very much WIP, but I'm trying to push the ball forward for #801 and #2141
Type: enhancement
Notes: Breaking change: Transition from deprecated/unsupported request library to fetch
Here's what your changelog entry will look like:
🚨 BREAKING CHANGES