[keycloak-dev] Promise wrapper for javascript adapter
Stian Thorgersen
sthorger at redhat.com
Mon Sep 25 07:36:41 EDT 2017
No, I'd be looking for a PR where keycloak.js uses standard promises
internally, but will return a wrapper for the promises that uses the legacy
api. There will be a config option that can be enabled to return standard
promises instead. After a few releases we make it return standard promises,
then finally after a few more releases we remove the legacy wrapper
completely.
On 22 September 2017 at 13:44, Raymond DeCampo <ray at decampo.org> wrote:
> So the next step is "deprecate and replace with standard"? So you would
> be looking for a pull request where keycloak.js is modified to return
> standard native Promises and the existing keycloak.js is moved to something
> like keycloak-legacy.js?
>
> On Wed, Sep 20, 2017 at 11:36 AM, Stian Thorgersen <sthorger at redhat.com>
> wrote:
>
>> +1 If we do promises that would be the only useful way to do it. Standard
>> promises wrapped to become wrapped. Have custom promises as default
>> initially. Deprecate and switch to standard after a while and finally
>> remove completely.
>>
>>
>> On 18 Sep 2017 6:16 pm, "Niels Bertram" <nielsbne at gmail.com> wrote:
>>
>> 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/b7af96aa4d7daeec6d
>>> a12ee9326d1c
>>> > 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