Skip to content
This repository has been archived by the owner on Mar 3, 2022. It is now read-only.

Use fetch instead of XMLHttpRequest #1335

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

mitar
Copy link

@mitar mitar commented Apr 15, 2021

XMLHttpRequest is not available inside service workers. One can always polyfill fetch onto XMLHttpRequest (there are many available) but I could not find any working polyfill for the opposite (polyfill XMLHttpRequest using fetch).

Also, I tried to use Oidc.Global.setXMLHttpRequest but I didn't really work, window is not defined so it is not returning set request, even when set. I think that check should be removed, or at least wrapped around || XMLHttpRequest check only.

TODO:

  • What to do about Oidc.Global.setXMLHttpRequest and get XMLHttpRequest? Should I change them to be about fetch?

See also: #1336

@kherock
Copy link

kherock commented Apr 30, 2021

+1 since it would help this library to run on the server as well. I think it should continue to read from Oidc.Global.fetch since server users could then use Oidc.Global.setFetch(require('node-fetch')) before initializing any OidcClient.

@brockallen
Copy link
Member

since it would help this library to run on the server as well

Thanks for the interest, but this library was never intended to run on the server. There are already others that fill this gap.

@sebastianrothe
Copy link

since it would help this library to run on the server as well

Thanks for the interest, but this library was never intended to run on the server. There are already others that fill this gap.

Actually fetch has nothing todo with server. It is browser-exclusive. Fetch is the modern replacement for xmlhttprequest. Even for node, you need a library (node-fetch). Only problem is IE11- doesn't support it. For IE11 support you need two polyfills: fetch and Promises.

@brockallen
Copy link
Member

Sure, but what's the benefit of changing if what's used is working (in the browser context since that's the only requirement)?

@mitar
Copy link
Author

mitar commented May 27, 2021

As I stated above, XMLHttpRequest is not available inside service workers. Service workers ARE in the browser context.

@sebastianrothe
Copy link

And IE11 is deprecated.

@brockallen
Copy link
Member

As I stated above, XMLHttpRequest is not available inside service workers. Service workers ARE in the browser context.

Ok, fair point. I don't know how the rest of the library will work in a service worker, tho.

@mitar
Copy link
Author

mitar commented May 28, 2021

Ok, fair point. I don't know how the rest of the library will work in a service worker, tho.

It works great with this PR. :-) We are using this fork ourselves in our Chrome browser extension using manifest 3 which has logic in service workers.

@sebastianrothe
Copy link

So, whats missing to merge it?

@brockallen
Copy link
Member

So, whats missing to merge it?

Me having time to review.

@mitar
Copy link
Author

mitar commented Jun 2, 2021

There is one TODO, maybe @brockallen can provide input on that before the review.

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

Successfully merging this pull request may close these issues.

4 participants