openid / AppAuth-JS

JavaScript client SDK for communicating with OAuth 2.0 and OpenID Connect providers.
Apache License 2.0
985 stars 161 forks source link

Replace Resolver based on jQuery.xhr with fetch. #48

Closed dwaite closed 6 years ago

dwaite commented 6 years ago
dwaite commented 6 years ago

Note: I do not know the copyright policy for the project (files are copyright Google, not OIDF) so I added myself as the copyright holder in the header for the new files.

geminiyellow commented 6 years ago

👍

tikurahul commented 6 years ago

Closing this PR given that this is no longer something we want to do.

jakefeasel commented 6 years ago

It would be very useful to have an alternative Requestor implementation that does not require jquery to be available globally. A fetch-based Requestor would be ideal.

tikurahul commented 6 years ago

Yes. It’s not that hard to build such an implementation. We realized that the typings for fetchs assumed a lot of stuff was global which makes it untenable as a interface whole implementation can be provided.

I think using jquery..xhr’s typings are much better suited. It should be easy to provide an implementation of the Requestor that uses fetch.

jakefeasel commented 6 years ago

Yes, I did this in my app by instantiating the BaseTokenRequestHandler like so:

new AppAuth.BaseTokenRequestHandler({
            // fetch-based alternative to built-in jquery implementation
            xhr: function (settings) {
                return new Promise(function (resolve, reject) {
                    fetch(settings.url, {
                        method: settings.method,
                        body: settings.data,
                        mode: 'cors',
                        cache: 'no-cache',
                        headers: settings.headers
                    }).then(function (response) {
                        if (response.ok) {
                            response.json().then(resolve);
                        } else {
                            reject(response.statusText);
                        }
                    }, reject);
                });
            }
        })

With this change, I was able to drop jQuery as a dependency. It would be great if this was an OOTB option.

tikurahul commented 6 years ago

Yes, this is exactly what I was talking about. I could provide another Requestor out of the box. Would that help?

jakefeasel commented 6 years ago

Yes, that would be very helpful. An even more neat trick would be to detect the presence / absence of $.ajax and use that to decide on what would be the best default Requestor.

tikurahul commented 6 years ago

Yes. That would be amazing.

alxmiron commented 6 years ago

Agree. jquery is not suitable for react-based apps

dwaite commented 6 years ago

I wrote this fetch-based version as part of a sample FWIW.

https://github.com/pingidentity/angular-spa-sample/blob/master/src/app/html5_requestor.ts

tikurahul commented 6 years ago

@dwaite Its easy to write something specific for your app / use-case. However there are more things one needs to consider when its a library which could be potentially used by a lot of diverse clients.