[keycloak-dev] Promise wrapper for javascript adapter
Raymond DeCampo
ray at decampo.org
Fri Sep 22 07:44:37 EDT 2017
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