[keycloak-dev] Promise wrapper for javascript adapter
Niels Bertram
nielsbne at gmail.com
Mon Sep 18 12:16:22 EDT 2017
Hmm ... I was actally looking at the js libraries (browser and node ones) a
short while ago and thought the same thing, why not use a A+ compliant
promise [1] instead of the custom thingy. It is hard to follow custom logic
when basic concepts exist and are understood.
Why not implement standard promises in the main library and write a wrapper
for the custom promises. Then mark those for deprecation and retire them in
a few releases. I think mongoose did this some time back with a pluggable
promise interface [2] to escape callback hell.
I think this would improve the adoptability of the adapters given lots of
folks don't fully understand OIDC and throw things in the too hard basket
as soon as they are given the slightest opportunity - real world
experience.
Cheers Niels
[1] https://promisesaplus.com
[2] http://mongoosejs.com/docs/promises.html
On 18 Sep. 2017 4:44 am, "Stian Thorgersen" <sthorger at redhat.com> wrote:
> I don't like the idea of having a wrapper as it needs updating separately
> when anything changes. If it's a different library I believe it'll just be
> forgotten about and also be annoying boilerplate to maintain. That's why
> I'd rather just have it return a different promise object depending on what
> config option is used. There's not that many places internally where
> promises objects are used, so we can probably find a reasonable way to do
> that.
>
> That being said I'm wondering if this is actually a case where this is just
> another way of doing something that won't actually be used. AFAIK no one
> else have asked for this nor have we seen anyone else comment on this
> thread. So maybe the best option here is to just leave it as is?
>
> On 13 September 2017 at 14:31, Raymond DeCampo <ray at decampo.org> wrote:
>
> > I understand the concern about increasing the test surface.
> >
> > However I don't think that introducing a configuration option and having
> > methods that return different objects based on the option will decrease
> the
> > testing risk. There are places in the keycloak.js where it invokes it's
> > own methods and uses the custom promise object, for example:
> >
> > https://github.com/keycloak/keycloak/blob/b7af96aa4d7daeec6da12ee9326d1c
> > 0fdcc4850f/adapters/oidc/js/src/main/resources/keycloak.js#L128-L131
> >
> >
> > kc.login(options).success(function () {
> > initPromise.setSuccess();
> > }).error(function () {
> > initPromise.setError();
> > });
> >
> >
> > So these places would have to change based on the configuration option
> and
> > I think it will become a bigger problem.
> >
> > What if instead we add a method on the Keycloak object which returns a
> > wrapper instance using Promises and keep everything in the one
> keycloak.js
> > file? So clients could do:
> >
> > var keycloak = new Keycloak().promiseWrapper();
> >
> > keycloak.init().then(function() { ... });
> >
> >
> > And everything is in the one file?
> >
> >
> > Although let me make one more pitch on the merits of my original
> > suggestion:
> >
> >
> > 1. No changes to keycloak.js => less risk of breaking something that
> > already works
> > 2. The KeycloakPromise implementation delegates to the keycloak.js
> > implementation which means that fixes to keycloak.js automatically
> > propogate.
> > 3. The KeycloakPromise implementation is a wrapper which means it only
> > needs to be changed if there is a change to the keycloak.js API, e.g.
> a new
> > method or a new argument to a method (but not a new option on an
> existing
> > argument)
> > 4. I think it will be clearer to document the interfaces for the end
> > user but of course that is a matter of opinion.
> >
> >
> >
> > On Tue, Sep 12, 2017 at 11:28 PM, Stian Thorgersen <sthorger at redhat.com>
> > wrote:
> >
> >> I can see how it would be nice. I'm a bit worried that the separate
> >> javascript file would simply get out of date and it would also need
> >> separate testing. We're already lacking a bit on testing of the JS
> adapter
> >> in the first place and this would be yet another thing to test. Is
> there a
> >> way we could simply have a config option in the current javascript
> adapter
> >> that allows setting what promise to return? That way it's probably just
> a
> >> few lines in keycloak.js where it's creating the promise objects to
> switch
> >> between the two?
> >>
> >> On 5 September 2017 at 05:58, Raymond DeCampo <ray at decampo.org> wrote:
> >>
> >>> First, if I were to integrate Promise support into the Keycloak, I
> would
> >>> be leaving the current javascript adapter as is. This would be an
> >>> extension in a separate javascript file so that only developers who are
> >>> interested would get the Promise interface. Existing code using the
> >>> current adapter would be unaffected. The new code would be a wrapper
> >>> around the existing code so that maintenance would only be required if
> the
> >>> javascript adapter API changes.
> >>>
> >>> In terms of browser support, caniuse.com[1] indicates that Promise has
> >>> broad support; only IE 11 and Opera Mini fail to support Promise.
> However
> >>> polyfills are available to provide support.
> >>>
> >>> As far as the advantages, the main ones as I see it are: 1) using the
> >>> standard promise API makes it easier for developers new to the
> library; 2)
> >>> using the standard promise API makes the library more interoperable
> with
> >>> other libraries; and 3) ES6 Promise objects are chainable making
> sequences
> >>> of asynchronous calls cleaner to code.
> >>>
> >>> I think the MDN page on Using Promises[2] probably makes a better
> >>> argument for than I'll be able to here. But the main thrust is that
> >>> chaining makes it so it is not necessary to have so many nested
> functions.
> >>>
> >>> For example, suppose I am writing a javascript client for a REST API
> >>> protected by Keycloak. Since the built-in fetch() method returns a
> Promise
> >>> with ES6 Promises I can write:
> >>>
> >>> // assume authz.updateToken() returns an ES6 Promise
> >>> authz.updateToken(5).then((token) => {
> >>> return fetch(this.ENDPOINT, {
> >>> redirect: 'error',
> >>> headers: { "Authorization": "Bearer " + token }
> >>> });
> >>> }).then((response) => {
> >>> showResult(response.text());
> >>> }).catch((error) => {
> >>> displayError(error);
> >>> });
> >>> }
> >>>
> >>> Since everything is standard, any JS developer would understand the
> >>> code. Since both libraries use Promise, I can chain the promises
> without
> >>> nesting functions. And since both libraries use Promise, I can use
> catch()
> >>> at the end for unified error handling whether the error comes from the
> >>> Keycloak adapter or from the call to fetch().
> >>>
> >>>
> >>> [1] http://caniuse.com/#search=Promise
> >>> [2] https://developer.mozilla.org/en-US/docs/Web/JavaScript/
> >>> Guide/Using_promises
> >>>
> >>>
> >>> On Tue, Sep 5, 2017 at 6:18 AM, Stian Thorgersen <sthorger at redhat.com>
> >>> wrote:
> >>>
> >>>> I don't know much about ES6 native Promises. Can you give me some
> >>>> details about it? What browser support it? What are the benefits of
> using
> >>>> that instead of our howegrown approach? Etc.
> >>>>
> >>>> On 4 September 2017 at 20:36, Raymond DeCampo <ray at decampo.org>
> wrote:
> >>>>
> >>>>> Hello all,
> >>>>>
> >>>>> I create a wrapper around the javascript adapter which uses ES6
> native
> >>>>> Promises. (https://github.com/RayDeCampo/keycloak-promise).
> >>>>>
> >>>>> If you are interested, I can re-package it for inclusion in Keycloak
> >>>>> and
> >>>>> create a pull request on GitHub. If you have a different process for
> >>>>> contributions, let me know.
> >>>>>
> >>>>> If you are not interested, no hard feelings.
> >>>>>
> >>>>> Thanks,
> >>>>> Ray
> >>>>> _______________________________________________
> >>>>> keycloak-dev mailing list
> >>>>> keycloak-dev at lists.jboss.org
> >>>>> https://lists.jboss.org/mailman/listinfo/keycloak-dev
> >>>>>
> >>>>
> >>>>
> >>>
> >>
> >
> _______________________________________________
> keycloak-dev mailing list
> keycloak-dev at lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/keycloak-dev
>
More information about the keycloak-dev
mailing list