[keycloak-dev] Promise wrapper for javascript adapter

[Raymond DeCampo]

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/b7af96aa4d7daeec6da12ee9326d1c0fdcc4850f/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
>>>>
>>>
>>>
>>
>