Re: [keycloak-dev] Promise wrapper for javascript adapter

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(a)decampo.org&gt; 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(a)redhat.com&gt; > 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(a)gmail.com&gt; 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(a)decampo.org&gt; 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(a)redhat.com&gt; 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(a)gmail.com&gt; 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(a)redhat.com&gt; >>>>> 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(a)decampo.org&gt; >>>>>> 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(a)redhat.com&gt; >>>>>> > 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(a)decampo.org&gt; >>>>>> 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(a)redhat.com&gt; >>>>>> >>> 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(a)decampo.org&gt; >>>>>> 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(a)lists.jboss.org >>>>>> >>>>> https://lists.jboss.org/mailman/listinfo/keycloak-dev >>>>>> >>>>> >>>>>> >>>> >>>>>> >>>> >>>>>> >>> >>>>>> >> >>>>>> > >>>>>> _______________________________________________ >>>>>> keycloak-dev mailing list >>>>>> keycloak-dev(a)lists.jboss.org >>>>>> https://lists.jboss.org/mailman/listinfo/keycloak-dev >>>>>> >>>>> >>>>> >>>> >>>