[keycloak-dev] Promise wrapper for javascript adapter
Raymond DeCampo
ray at decampo.org
Mon Sep 25 21:33:07 EDT 2017
I think maybe we are making this too complicated. Since this is JS, we
could take a native Promise instance and add the methods defined by the
existing promise return value. Then it would work using the existing
callbacks or using standard Promise methods.
Something like:
if (typeof Promise === "function") {
var promise = new Promise(...);
promise.success = function(callback) { ... };
promise.error = function(callback) { ... };
} else {
// return existing custom promise object
}
On Mon, Sep 25, 2017 at 7:36 AM, Stian Thorgersen <sthorger at redhat.com>
wrote:
> 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:
>>>
>>>> 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#L
>>>>>>> 128-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