KEYCLOAK-11195 Add module loading to dependencies

[karelhala]

This PR will add 2 new dependencies and when this package is installed it will also install these dependencies. If you are going to use the JS adapter in module based system these dependencies will be loaded from node_modules, if you are going to use just the JS file it will use these dependencies from window.

Huge kudos goes to webpack#externals and their library module building.

[stianst]

Thanks for the PR. I have some concerns around this:

• Potential different versions of libraries
• More testing as we need to somehow test both approaches
• Not sure how we can create RH-SSO JS adapter. Not sure if we have what we need to distribute the dependencies when not "embedded" in keycloak.js

[karelhala]

Thanks for the PR. I have some concerns around this:

• Potential different versions of libraries

They are in strict version in package.json, even if consumer is going to install their own version of dependencies npm will use these because the way how npm resolves direct dependencies. You can check this by running npm list you'd see that this has direct dependency on specific version.

• More testing as we need to somehow test both approaches

I'll be honest with you that I don't have the skill enough to write tests for this in Java, if someone would be willing to help me that would be great. I could write unit tests, but given that integration tests for this approach were missing even before this PR was opened is it really required to write them here. As I said, I cal take a look into it, but it will take a lot of time.

• Not sure how we can create RH-SSO JS adapter. Not sure if we have what we need to distribute the dependencies when not "embedded" in keycloak.js

If you are going to ship this file only as .js file these dependencies will be included, but if someone is going to install this package over npm they'll get these dependencies covered by npm.

[ibauersachs]

In the npm/webpack case, doesn't this end up with loading (i.e. transferring to the client, not actually interpreting) the two libraries twice? A proper npm package and a separate "uber-js" version for the version shipped/served by Keycloak directly seems somewhat more appropriate.

[jonkoops]

Just chipping in here, I think there are two separate issues at hand here. The first being that there is now a version of Keycloak distributed to users that is broken and the second being that a proper system should be introduced for new dependencies of keycloak-js that works both for consumers that use a build system as well as users that just include the script statically.

Although this is a great improvement I would recommend we take a phased approach to fixing the problems.

First of all I would recommend unpublishing version 7.0.0 from NPM as this is bound to create only confused users that will pile onto this issue. @stianst I believe you are in a postion to do so if I am not mistaken, documentation can be found here.

Secondly let's narrow down this PR to just ensuring the problem at hand, which is to remove the module code that is causing the problem. After this we can publish a new version under the same number that should work properly.

[karelhala]

In the npm/webpack case, doesn't this end up with loading (i.e. transferring to the client, not actually interpreting) the two libraries twice?

Not really, dead code analysis and treeshakig will remove the last if statement and you'd end up with just the required version.

A proper npm package and a separate "uber-js" version for the version shipped/served by Keycloak directly seems somewhat more appropriate.

I think that KC team is working towards this system, but this is just to fix broken version in NPM, I would love to see full JS module and everything for this adapter, but that is harder task and as I said this PR is just to fix exports.

being that a proper system should be introduced for new dependencies of keycloak-js that works both for consumers that use a build system as well as users that just include the script statically.

That is great idea, but I don't think in scope with this PR. This PR aims only to properly load libraries and export correctly for all modules.

First of all I would recommend unpublishing version 7.0.0

100 % agree with you.

Secondly let's narrow down this PR to just ensuring the problem at hand, which is to remove the module code that is causing the problem.

I am affraid that this is way harder than just remove the problematic code. You see this would require removing features that might already be used generatePkceChallenge function and also change minified code of 3rd party library. I tried several ways and all seemed hacky (mostly because we'd have to somehow access functions from base64-js and js-sha256), this is actually pretty elegant and narrow way, that suits all use cases.

[jonkoops]

@karelhala I manged to get base64 down by removing all the module related code from it an turning it into a factory function. sha256 seems a lot harder as it has hand-rolled module code bit I think I can work it out. Using a similar pattern to your PR we should be able to construct the modules without them registering themselves outside of the context of Keycloak and without the need to load any additional code for Webpack builds.

[karelhala]

@jonkoops just to be clear, this is not for webpack only sollution, it's standard (both for UMD and AMD) module loading and exporting with exception to static file when using window because KC has no build for JS sadly.

I tried messing with the module.exports as well, but figured that it's not good and honeslty I felt dirty by changing minified 3rd party library. You have to note that what if KC wants to add some other library with approach in this PR it's easy by adding require in the first part. Plus if there is need for updating libraries that are already used we'd have to mimic what you are trying to achieve and it is bug prone approach.

Is there any reason why you don't like requirig external dependencies as proposed in this PR?

[jonkoops]

@karelhala Yeah, I agree I am also not a fan of modifying the minified scripts but I see no better options at this point. My concerns are basically the same as those that @stianst mentioned before.

[karelhala]

Restarting build.

[jonkoops]

@karelhala Did some quick testing with both a simple Webpack build and using the static file directly and it's working perfectly. I've also checked if the unused code is getting eliminated properly in the Webpack version and this is also working as expected!

[jonkoops]

Other than the changes in the module code that might affect a small subset of users I do not see any problems merging this PR 👍

[jonkoops]

In the original code the global for Keycloak is registered in all cases where module.exports does not exist. This means that even if AMD is present the Keycloak global is still exposed (see the original code).

This also means that we cannot assume that people that are using the static file in combination with AMD are not using the Keycloak global, or have the dependencies js-sha256 and base64-js included.

[karelhala]

This means that even if AMD is present the Keycloak global is still exposed

Good point! Will update it to be part of window as well.

or a have the dependencies js-sha256 and base64-js included

Mhh yeah, you are correct. I often forget that you have to actually create these files with AMD. I'll update the code so it will pass these dependencies from window.

[jonkoops]

All looks good to me, @mhajas I think we're good to go here.

[mhajas]

Thank you very much @jonkoops for the review, we really appreciate it. @thomasdarimont Could you please do a review as well?

Unfortunately, today I am overwhelmed with another task. I hope tomorrow or on Friday at the latest I will get to this.

[mhajas]

@karelhala Could you please squash commits?

[mhajas]

I did some testing and it is working as expected. Thank you very much to all of you. @stianst Should we publish the new version of the module when this Pr is merged? Or we need to wait until a new version of keycloak is released?

[jonkoops]

Perhaps it would be wise to cherry-pick this change on top of the last release tag and manually re-publish version 7.

[sassko]

Hi,

it seems that these comments that are committed within this PR into the keycloak.js

leads to some issues with the minified version:

Source map error: SyntaxError: JSON.parse: unexpected character at line 2 column 1 of the JSON data
Resource URL: https://DOMAIN/auth/js/keycloak.min.js
Source Map URL: keycloak.min.js.map

I don't know it is worth to create an issue, just want to mention it.

[jonkoops]

@sassko If you can create a reproducible case of this please report the bug in the issue tracker.

[jonkoops]

@stianst @abstractj It's been a while since this PR has been merged and I know people are waiting on this fix to be released so that they can make use of new functionality introduced in Keycloak v7.

Personally I would also like to start migrating angular-keycloak to the Promise based API and introduce silent SSO, all of these features only work in v7.

Would it be possible to get an intermediate release just for Keycloak JS so that we can proceed?

edit I've sent an e-mail to the Keycloak dev mailing list formally requesting a new release.
edit Keycloak 7.0.1 is now available!

[jonkoops]

FYI it looks like Keycloak 7.0.1 has introduced a regression in the TypeScript definitions causing it to incorrectly assume native promises are used by default. This issue has already been resolved (bc5b4de), but for some reason was not included in the release of 7.0.1.

If you would like to update to the latest version of Keycloak and you are using TypeScript make sure to specify that you want to use native promises instead of Keycloak specific ones. For more information on how to do this see this PR to the documentation: keycloak/keycloak-documentation#742