[keycloak-dev] Promise wrapper for javascript adapter
Stian Thorgersen
sthorger at redhat.com
Mon Sep 18 10:02:07 EDT 2017
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
>>>>>
>>>>
>>>>
>>>
>>
>
More information about the keycloak-dev
mailing list