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

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 >> > >

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(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 >>>> >>> >>> >> >

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/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(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

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#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(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 >> > >

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(a)redhat.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: >>> >>>> 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 >>>>>>> >>>>>> >>>>>> >>>>> >>>>