search

IdentityModel / oidc-client-js

OpenID Connect (OIDC) and OAuth2 protocol support for browser-based JavaScript applications
Apache License 2.0
2.43k stars 842 forks source link

Use fetch instead of XMLHttpRequest #1335

Open mitar opened 3 years ago

mitar commented 3 years ago

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:

See also: https://github.com/IdentityModel/oidc-client-js/issues/1336

kherock commented 3 years ago

+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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

sebastianrothe commented 3 years ago

And IE11 is deprecated.

brockallen commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

So, whats missing to merge it?

brockallen commented 3 years ago

So, whats missing to merge it?

Me having time to review.

mitar commented 3 years ago

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