oidc-client-js icon indicating copy to clipboard operation
oidc-client-js copied to clipboard

Use fetch instead of XMLHttpRequest

Open mitar opened this issue 3 years ago • 11 comments

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: https://github.com/IdentityModel/oidc-client-js/issues/1336

mitar avatar Apr 15 '21 08:04 mitar

+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.

kherock avatar Apr 30 '21 00:04 kherock

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.

brockallen avatar Apr 30 '21 00:04 brockallen

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.

sebastianrothe avatar May 27 '21 11:05 sebastianrothe

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

brockallen avatar May 27 '21 12:05 brockallen

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

mitar avatar May 27 '21 14:05 mitar

And IE11 is deprecated.

sebastianrothe avatar May 27 '21 21:05 sebastianrothe

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.

brockallen avatar May 27 '21 22:05 brockallen

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.

mitar avatar May 28 '21 00:05 mitar

So, whats missing to merge it?

sebastianrothe avatar Jun 02 '21 14:06 sebastianrothe

So, whats missing to merge it?

Me having time to review.

brockallen avatar Jun 02 '21 14:06 brockallen

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

mitar avatar Jun 02 '21 16:06 mitar