Thanks for your feedback, some comments below:
On Wed, 21 Nov 2018, 13:26 Guillaume Vincent <gvincent(a)redhat.com wrote:
Hello,
this mail is an email to give my feedback on Keycloak Javascript Adapter.
The feedback is not good. If you are not open to criticism, do not read
this email.
TLDR: The Javascript code is a 1500+ line of code, untested, with 10 levels
of indentations, no abstraction, difficult to read, difficult to maintain,
include in an web application and not following semver.
# loc
maintain and fix 1500 line of code is not easy at all. The javascript
adapter should be split in small javascript functions/modules/class and
respect some basic clean code. SRP, no duplication, etc.
Breaking up also introduces some complexity so I'm not fully convinced.
# tests
No single test on the javascript adapter? really. We should introduce some
basic tests and API test to avoid breaking the interface. Basically
decrypting a JWT, parsing the url, initialisation could be tested easily.
Of course there are tests. Tests are part of our Arquillian Java testsuite
though.
# semver
In javascript world, the common convention regarding the version number is
semver[1]. So when the API change, the major version should be incremented.
Not the minor one. We could avoid like that fixing the version number in
package.json [2] to avoid those kind of bug [3]
It's not quite that trivial as we release everything in Keycloak as a
bundle. We can't bump the major version for a small api change and I know
which one you are referring to. We strive to not break APIs and rather use
deprecation. However, in the case you are referring to this was not
possible as it was a broken design and there was no way to fix it. Api
hasn't really changed either we just require you to set a config option to
use native promises.
# integration in existing app
nobody today create real professional frontend application using the plain
javascript file. The frontend world is compose of small piece assemble
together. We use bundler like webpack, browserify, rollup etc to create our
application. Basically we don't do this:
```
<script src="keycloak.js"></script>
<script>
var keycloak = Keycloak();
keycloak.init()
</script>
```
we do this
```
const Keycloack = require("keycloack");
var keycloak = Keycloak();
keycloak.init();
```
yes like in node
Yes, some do, but not everyone. The adapter has been designed with the aim
to work for both cases.
# modules
the keycloak library should use modules or be decomposed into modules.
Decoupled in small piece and use rollup or webpack to bundle the
application. It will help for example bundler to do tree shacking for
example and reduce the bundle size of the library.
So instead of try to reinvent the wheel and introduce some insecure
code[4]. We should use javascript module, well tested like this one[5] and
do something like
const uuidv4 = require('uuid/v4');
function createUUID() {
return uuidv4();
Perhaps, but using libraries has its own problems. End of the day it's
easier for us to support an in-line uiid function than support external
libraries.
# hashtag
redirectURL doesn't work if it contains a hashtag. Popular framework
AngularJS, VueJS use hastag technique for routing.
It does. Try the admin console for instance which is angular and uses
hashtags
# cookies, iframe
This make our work as developer trully hard. Basically the simplest
javascript adpater, should be
Instantiate a keycloak singleton
load existing jwt in localstorage if present
load url params to see if jwt in the url
if jwt is present, save in local storage
update the singleton
Don't know what you are talking about here really. There's no cookies and
the only iframe is the session iframe and there's no other way to do that
# cordova adapter
I don't look at this code, but I'm just asking myself, why a javascript
adapter need a special code for cordova application?
Cordova mode opens login page in web view or browser tab, regular mode
doesn't.
# keycloak promise Oo
https://www.keycloak.org/docs/latest/upgrading/index.html#javascipt-adapt...
A promise is standardized in javascript, so you should avoid using this
term in the code if you don't return a promise. callback is a better term.
A promise should return a unfulfilled method with then and catch methods
attach to it.
We have both. Look at docs and upgrade guide.
In French we have a proverb: "Criticism is easy, art is difficult". So I
will definitely create a smaller javascript adapter that does not suffer
from the problems listed above. And I would come and show the code here.
Thanks for reading me
[1]
https://semver.org/
[2]
https://github.com/redhat-cip/dci-ui/commit/e4d0208128afa05c33e72abe606cd...
[3]
https://issues.jboss.org/browse/KEYCLOAK-8839
[4]
https://github.com/keycloak/keycloak/blob/master/adapters/oidc/js/src/mai...
[5]
https://github.com/kelektiv/node-uuid
--
Guillaume Vincent
Senior FrontEnd Engineer
gvincent(a)redhat.com
<
https://red.ht/sig>
TRIED. TESTED. TRUSTED. <
https://redhat.com/trusted>
_______________________________________________
keycloak-dev mailing list
keycloak-dev(a)lists.jboss.org
https://lists.jboss.org/mailman/listinfo/keycloak-dev