Jon Koops <jonkoops(a)gmail.com&gt; hat am 19. September 2019 um 13:12 geschrieben: Hi all, I've created some pull requests and tasks for moving towards standardized Promises instead of Keycloak's own custom implementation in order to start a discussion about how to move forward. There has already been some discussion about plans to migrate away from the current situation in KEYCLOAK-9346 <https://issues.jboss.org/browse/KEYCLOAK-9346>;. I believe the first order of business to start migrating towards a better place would be to start informing Keycloak's users about the change. In order to do so I propose to make the following changes: 1. The documentation of the JavaScript Adapter should reflect the preference of using the native promises. We can accomplish this by updating the examples used and providing additional documentation of the promiseType field. 2. Users that are currently using Keycloak should be informed that standardized promises are preferred and that they should move away from the current implementation. In order to do so a deprecation warning should be logged in the console with instructions on how to migrate their existing code. I've created the following issues and pull requests for this: *Updates to the documentation preferring promiseType native* - https://github.com/keycloak/keycloak-documentation/pull/742 - https://issues.jboss.org/browse/KEYCLOAK-11436 *Updates to the adapter to inform users of the deprecation* - https://github.com/keycloak/keycloak/pull/6318 - https://issues.jboss.org/browse/KEYCLOAK-11435 Furthermore I would like to start a discussion on these changes and how to move forward after. Feedback on this is greatly appreciated! Regards, Jon _______________________________________________ keycloak-dev mailing list keycloak-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/keycloak-dev

Updated https://issues.jboss.org/browse/KEYCLOAK-9346 with sub-tasks to cover what we agreed and moved your two tasks to sub-tasks of this issue. I think both your PRs can be merged now, with https://issues.jboss.org/browse/KEYCLOAK-11608 covering a clearer message around the deprecation. I can handle that, but need to discuss with the team around how we go about this first. Not sure when we can make this switch, will discuss it with the team on Friday and let you know. On Fri, 27 Sep 2019 at 13:58, Jon Koops <jonkoops(a)gmail.com&gt; wrote: > Great! I completely agree that the points outlined are the way forward. > Let's focus on getting the deprecation message into the next release > together with the updated documentation. There are pull requests for these > changes and I would greatly appreciate a review of them. > > I'll also modify the existing pull requests to incorporate the idea of a > 'legacy' promise type and refer to it the documentation with some > elaboration that in a future version of Keycloak the default will change to > 'native' over 'legacy'. > > As for the timeline of the changing of the default to 'native' and > eventual removal of 'legacy' what would be the preferred moment to make > these changes? >

On Wed, 2 Oct 2019 at 13:37, Jon Koops <jonkoops(a)gmail.com&gt; wrote: > As far as I am concerned the pull requests can be merged as-is. However > we need to make sure to add a 'legacy' promiseType to cover the transition > period where promiseType will default to 'native' to give users the ability > to opt-out of the native promises whilst migrating their code. > > Depending on what you prefer we can do two things. We either merge the > pull requests and make a new task to introduce a 'legacy' promiseType. Or I > can update the pull requests that are open now. Let me know what you think. > Added https://issues.jboss.org/browse/KEYCLOAK-11616 - if you can incorporate that in current PRs that would be great. > > Related to this discussion there are some files that seem duplicated as I > can find the same type definitions for the Keycloak adapter in the new > theme as almost an exact copy. What should be done with with these files if > we make changes? (the specific file in question is > themes/src/main/resources/theme/keycloak-preview/account/resources/app/keycloak-service/keycloak.d.ts) > > It also seems that in the respective classes that use said definition > that the legacy promises are also still used. We should look into finding a > way to migrate this code as well. Perhaps including Keycloak as an NPM > dependency would make some sense here as well. > This is there temporarily for the new account console and the team working on the new account console is already aware that they need to remove this duplicate. > > I don't consider the above observation as part of the migration in > question, however it should probably be discussed by the team as well. > > > On Wed, Oct 2, 2019 at 10:54 AM Stian Thorgersen <sthorger(a)redhat.com&gt; > wrote: > >> Updated https://issues.jboss.org/browse/KEYCLOAK-9346 with sub-tasks to >> cover what we agreed and moved your two tasks to sub-tasks of this issue. >> >> I think both your PRs can be merged now, with >> https://issues.jboss.org/browse/KEYCLOAK-11608 covering a clearer >> message around the deprecation. I can handle that, but need to discuss with >> the team around how we go about this first. >> >> Not sure when we can make this switch, will discuss it with the team on >> Friday and let you know. >> >> On Fri, 27 Sep 2019 at 13:58, Jon Koops <jonkoops(a)gmail.com&gt; wrote: >> >>> Great! I completely agree that the points outlined are the way forward. >>> Let's focus on getting the deprecation message into the next release >>> together with the updated documentation. There are pull requests for these >>> changes and I would greatly appreciate a review of them. >>> >>> I'll also modify the existing pull requests to incorporate the idea of >>> a 'legacy' promise type and refer to it the documentation with some >>> elaboration that in a future version of Keycloak the default will change to >>> 'native' over 'legacy'. >>> >>> As for the timeline of the changing of the default to 'native' and >>> eventual removal of 'legacy' what would be the preferred moment to make >>> these changes? >>> >>

I've incorporated the code with the changes we discussed and it can be reviewed. See: https://github.com/keycloak/keycloak/pull/6318. Still need to update the documentation I'll get to that hopefully tomorrow, if not the day after. On Wed, Oct 2, 2019 at 1:57 PM Jon Koops <jonkoops(a)gmail.com&gt; wrote: > Thanks for the quick response. I'll see if I can get the changes in today. > > On Wed, Oct 2, 2019 at 1:50 PM Stian Thorgersen <sthorger(a)redhat.com&gt; > wrote: > >> >> >> On Wed, 2 Oct 2019 at 13:37, Jon Koops <jonkoops(a)gmail.com&gt; wrote: >> >>> As far as I am concerned the pull requests can be merged as-is. However >>> we need to make sure to add a 'legacy' promiseType to cover the transition >>> period where promiseType will default to 'native' to give users the ability >>> to opt-out of the native promises whilst migrating their code. >>> >>> Depending on what you prefer we can do two things. We either merge the >>> pull requests and make a new task to introduce a 'legacy' promiseType. Or I >>> can update the pull requests that are open now. Let me know what you think. >>> >> >> Added https://issues.jboss.org/browse/KEYCLOAK-11616 - if you can >> incorporate that in current PRs that would be great. >> >> >>> >>> Related to this discussion there are some files that seem duplicated as >>> I can find the same type definitions for the Keycloak adapter in the new >>> theme as almost an exact copy. What should be done with with these files if >>> we make changes? (the specific file in question is >>> themes/src/main/resources/theme/keycloak-preview/account/resources/app/keycloak-service/keycloak.d.ts) >>> >>> It also seems that in the respective classes that use said definition >>> that the legacy promises are also still used. We should look into finding a >>> way to migrate this code as well. Perhaps including Keycloak as an NPM >>> dependency would make some sense here as well. >>> >> >> This is there temporarily for the new account console and the team >> working on the new account console is already aware that they need to >> remove this duplicate. >> >> >>> >>> I don't consider the above observation as part of the migration in >>> question, however it should probably be discussed by the team as well. >>> >>> >>> On Wed, Oct 2, 2019 at 10:54 AM Stian Thorgersen <sthorger(a)redhat.com&gt; >>> wrote: >>> >>>> Updated https://issues.jboss.org/browse/KEYCLOAK-9346 with sub-tasks >>>> to cover what we agreed and moved your two tasks to sub-tasks of this issue. >>>> >>>> I think both your PRs can be merged now, with >>>> https://issues.jboss.org/browse/KEYCLOAK-11608 covering a clearer >>>> message around the deprecation. I can handle that, but need to discuss with >>>> the team around how we go about this first. >>>> >>>> Not sure when we can make this switch, will discuss it with the team >>>> on Friday and let you know. >>>> >>>> On Fri, 27 Sep 2019 at 13:58, Jon Koops <jonkoops(a)gmail.com&gt; wrote: >>>> >>>>> Great! I completely agree that the points outlined are the way >>>>> forward. Let's focus on getting the deprecation message into the next >>>>> release together with the updated documentation. There are pull requests >>>>> for these changes and I would greatly appreciate a review of them. >>>>> >>>>> I'll also modify the existing pull requests to incorporate the idea >>>>> of a 'legacy' promise type and refer to it the documentation with some >>>>> elaboration that in a future version of Keycloak the default will change to >>>>> 'native' over 'legacy'. >>>>> >>>>> As for the timeline of the changing of the default to 'native' and >>>>> eventual removal of 'legacy' what would be the preferred moment to make >>>>> these changes? >>>>> >>>>

Great, thanks. Will get some folks that know TypeScript better than me to take a look at it. On Wed, 2 Oct 2019 at 20:38, Jon Koops <jonkoops(a)gmail.com&gt; wrote: > I've incorporated the code with the changes we discussed and it can be > reviewed. See: https://github.com/keycloak/keycloak/pull/6318. Still > need to update the documentation I'll get to that hopefully tomorrow, if > not the day after. > > On Wed, Oct 2, 2019 at 1:57 PM Jon Koops <jonkoops(a)gmail.com&gt; wrote: > >> Thanks for the quick response. I'll see if I can get the changes in >> today. >> >> On Wed, Oct 2, 2019 at 1:50 PM Stian Thorgersen <sthorger(a)redhat.com&gt; >> wrote: >> >>> >>> >>> On Wed, 2 Oct 2019 at 13:37, Jon Koops <jonkoops(a)gmail.com&gt; wrote: >>> >>>> As far as I am concerned the pull requests can be merged as-is. >>>> However we need to make sure to add a 'legacy' promiseType to cover the >>>> transition period where promiseType will default to 'native' to give users >>>> the ability to opt-out of the native promises whilst migrating their code. >>>> >>>> Depending on what you prefer we can do two things. We either merge the >>>> pull requests and make a new task to introduce a 'legacy' promiseType. Or I >>>> can update the pull requests that are open now. Let me know what you think. >>>> >>> >>> Added https://issues.jboss.org/browse/KEYCLOAK-11616 - if you can >>> incorporate that in current PRs that would be great. >>> >>> >>>> >>>> Related to this discussion there are some files that seem duplicated >>>> as I can find the same type definitions for the Keycloak adapter in the new >>>> theme as almost an exact copy. What should be done with with these files if >>>> we make changes? (the specific file in question is >>>> themes/src/main/resources/theme/keycloak-preview/account/resources/app/keycloak-service/keycloak.d.ts) >>>> >>>> It also seems that in the respective classes that use said definition >>>> that the legacy promises are also still used. We should look into finding a >>>> way to migrate this code as well. Perhaps including Keycloak as an NPM >>>> dependency would make some sense here as well. >>>> >>> >>> This is there temporarily for the new account console and the team >>> working on the new account console is already aware that they need to >>> remove this duplicate. >>> >>> >>>> >>>> I don't consider the above observation as part of the migration in >>>> question, however it should probably be discussed by the team as well. >>>> >>>> >>>> On Wed, Oct 2, 2019 at 10:54 AM Stian Thorgersen <sthorger(a)redhat.com&gt; >>>> wrote: >>>> >>>>> Updated https://issues.jboss.org/browse/KEYCLOAK-9346 with sub-tasks >>>>> to cover what we agreed and moved your two tasks to sub-tasks of this issue. >>>>> >>>>> I think both your PRs can be merged now, with >>>>> https://issues.jboss.org/browse/KEYCLOAK-11608 covering a clearer >>>>> message around the deprecation. I can handle that, but need to discuss with >>>>> the team around how we go about this first. >>>>> >>>>> Not sure when we can make this switch, will discuss it with the team >>>>> on Friday and let you know. >>>>> >>>>> On Fri, 27 Sep 2019 at 13:58, Jon Koops <jonkoops(a)gmail.com&gt; wrote: >>>>> >>>>>> Great! I completely agree that the points outlined are the way >>>>>> forward. Let's focus on getting the deprecation message into the next >>>>>> release together with the updated documentation. There are pull requests >>>>>> for these changes and I would greatly appreciate a review of them. >>>>>> >>>>>> I'll also modify the existing pull requests to incorporate the idea >>>>>> of a 'legacy' promise type and refer to it the documentation with some >>>>>> elaboration that in a future version of Keycloak the default will change to >>>>>> 'native' over 'legacy'. >>>>>> >>>>>> As for the timeline of the changing of the default to 'native' and >>>>>> eventual removal of 'legacy' what would be the preferred moment to make >>>>>> these changes? >>>>>> >>>>>