[keycloak-dev] Merging node_modules into Keycloak repo

Fri May 12 04:45:49 EDT 2017

On 12 May 2017 at 10:25, Bruno Oliveira <bruno at abstractj.org> wrote:

> On 2017-05-11, Stan Silvert wrote:
> > On 5/11/2017 4:06 PM, Bruno Oliveira wrote:
> > > Some answers inline.
> > More comments inline...
> > >
> > > On 2017-05-11, Stian Thorgersen wrote:
> > > > Oh and also I like to just run stuff from the IDE. I very rarely
> build
> > > > Keycloak from mvn unless I'm mocking about with the distribution
> parts.
> > > To run stuff from your IDE, you just run npm install inside the
> project.
> > > You don't need Maven for it.
> > >
> > > > On 11 May 2017 at 19:17, Stian Thorgersen <sthorger at redhat.com>
> wrote:
> > > >
> > > > > I have 3 concerns around not checking in node_modules. If these are
> > > > > addressed I'm happy:
> > > > >
> > > > > * Productization - I believe not checking in the modules will mean
> they
> > > > > have to be available in some internal NPM repository or something.
> Those
> > > > > module not already available we'll have to add ourselves. Stan you
> would
> > > > > have to figure this out as we no longer have a dedicated prod team
> (that's
> > > > > us now!)
> > > Yep, I know. One of the reasons why run diff at each upgrade of these
> > > dependencies will sound like hell.
> > I don't know the answer to the productization question.  SOMEBODY in
> > MiddleWare
> > must have dealt with this before.  Who can I ask?
>
> I just asked Node team on it. Will share the answer as soon as I hear
> something back.
>
> > >
> > > > > * Build time - just the basic Angular2 example adds overhead
> > > I won't take the road of Java vs Node.js. But pretty much you have the
> > > same issue with Maven, when you don't have dependencies installed.
> > > One way or another you have to download it from the internet.
> > With Maven, you only have to download once.  But with node_modules not
> > checked in
> > you will have to download every time after you do a clean.  Or is there
> some
> > way around that?
>
> You have to download once, per project.
>

Where is it saved? I might be mistake, but I think MVN plugins for this
stuff just puts it in target dir, which is deleted on a mvn clean

>
> >
> > npm doesn't seem to have the same concept of a local repo like Maven.
> > Installing globally doesn't fix
> > the problem.  Or is there a solution I don't know about?
>
> The local repo lives inside the project. But no, there's no concept of
> global module for project dependencies and as the author states,
> that never gonna happen with npm (https://github.com/npm/npm/
> issues/2949#issuecomment-11408461).
>

That's just plain out stupid.... Mvn repo in home dir is one of it's
greatest concepts.

>
> > >
> > > > > * Offline mode - the Angular2 example at the moment can't be built
> when
> > > > > not online. It will sit around for a loooong time until it
> eventually times
> > > > > out. NPM modules would have to be checked in somewhere globally
> (not just
> > > > > inside target dir or something like that). Maybe they can just be
> placed
> > > > > inside the .m2 dir or something?!
> > > Isn't the same when you are disconnected and you try to run Maven?
> > No.   You can rely on the local Maven repo.  Works great disconnected.
>
> If you never downloaded any dependency, I doubt it ;)
>

As I said you just run mvn build once, then you can use it offline for as
long as you want unless dependencies change.

>
> > >
> > > Anyways, I don't think we gonna reach any consensus about it. So,
> anything
> > > you guys think is the best for the project, go ahead. It's software, we
> > > can always change, sometimes break :)
> > >
> > > > > On 11 May 2017 at 15:10, Bruno Oliveira <bruno at abstractj.org>
> wrote:
> > > > >
> > > > > > On 2017-05-11, Stan Silvert wrote:
> > > > > > > On 5/10/2017 6:14 PM, Bruno Oliveira wrote:
> > > > > > > > On 2017-05-10, Stan Silvert wrote:
> > > > > > > > > The very thought of $subject seems like heresy.  Why check
> in
> > > > > > something
> > > > > > > > > that is normally pulled using npm?
> > > > > > > > >
> > > > > > > > > We have Angular 2 examples in Keycloak now.  In the
> not-too-distant
> > > > > > > > > future, our Account Management console will be written in
> Angular
> > > > > > 2.  So
> > > > > > > > > node_modules has to be there somehow.
> > > > > > > > >
> > > > > > > > > There are basically two options:
> > > > > > > > > 1)  Merge node_modules into the Keycloak repo.
> > > > > > > > > 2)  Don't merge and then run npm install at build time.
> > > > > > > > >
> > > > > > > > > Productization standards push toward option #1.  We need
> to have
> > > > > > > > > consistent, repeatable builds.
> > > > > > > > >
> > > > > > > > > But I'm looking for reasons that #1 might be bad.  I can't
> come up
> > > > > > with
> > > > > > > > > a rational reason to do #2 except that it saves disk space.
> > > > > > > > My 2 cents here. I think #1 is bad for the following reasons:
> > > > > > > My instinct is that it's bad too.  But I need to play devil's
> advocate.
> > > > > > > > 1. That's not the convention for Node.js development. Think
> about Java
> > > > > > > > devs committing JARs to our repo. Certainly that would be
> terrible.
> > > > > > > That's just the heresy argument.
> > > > > > Not really. It's also the way which QE thinks is the way to go (
> > > > > > https://github.com/keycloak/keycloak-quickstarts/pull/25#is
> > > > > > suecomment-300751341)
> > > > > >
> > > > > > > > 2. Code review becomes a PITA at every dependency update.
> > > > > > > If you are talking about review in GitHub, that shouldn't be a
> problem.
> > > > > > > GitHub uses linguist to hide vendor files so that it doesn't
> mess up
> > > > > > your
> > > > > > > review.  All you see is the file name.  By default, it doesn't
> show you
> > > > > > the
> > > > > > > whole file.  Strangely enough, the linguist guys see checking
> in js
> > > > > > > libraries as a common practice:
> > > > > > > https://github.com/github/linguist#vendored-code
> > > > > > Well, you're assuming that we should all do our code reviews
> using only
> > > > > > GH.
> > > > > >
> > > > > > > > 3. Merge conflicts. Just think about multiple devs
> contribution to the
> > > > > > > > same repo and updating their modules.
> > > > > > > Maybe I'm missing something here.  Wouldn't that be really
> rare if it
> > > > > > ever
> > > > > > > happened at all (in our case)?
> > > > > > Not if every developer decide to run npm update :)
> > > > > >
> > > > > > > > 4. People could manually change these modules and you would
> never know
> > > > > > > > if the change is a consequence of `npm update` or a manual
> change.
> > > > > > > I really hope nobody would be dumb enough to do that!
> > > > > > >
> > > > > > > But come to think of it, this would be a security issue if
> someone did
> > > > > > it on
> > > > > > > purpose.   Someone could sneak malicious code into our repo
> disguised
> > > > > > as a
> > > > > > > legit PR.  It wouldn't be easy to catch during code review.
> > > > > > Trust me, if people wanted to do that. They wouldn't be dumb just
> > > > > > a single line
> > > > > >
> > > > > > > > 5. This only makes sense on scenarios where dev cannot rely
> on npm
> > > > > > > > dependencies.
> > > > > > > >
> > > > > > > >
> > > > > > > > IMO option #2 would be the most viable. Projects from RedHat
> already
> > > > > > do
> > > > > > > > this[1] with some success. So I don't see any need for it.
> > > > > > > >
> > > > > > > > [1] - https://github.com/aerogear/
> aerogear-unifiedpush-server/blob
> > > > > > /02b133ffb49677effa347788c28c392ed3f275f0/admin-ui/pom.xml#L42
> > > > > > > Doesn't this make the build a LOT slower?
> > > > > > If slowness is the major concern, we should commit every single
> JAR
> > > > > > inside the repo and call it a day. But I don't think we do that,
> right?
> > > > > > Plus, Travis can cache the dependencies[1].
> > > > > >
> > > > > > [1] - https://docs.travis-ci.com/user/caching/
> > > > > >
> > > > > > > How is Aerogear dealing with productization?
> > > > > > You have to ask them, more precisely Matthias.
> > > > > >
> > > > > > > >
> > > > > > > > > Any thoughts?
> > > > > > > > > _______________________________________________
> > > > > > > > > keycloak-dev mailing list
> > > > > > > > > keycloak-dev at lists.jboss.org
> > > > > > > > > https://lists.jboss.org/mailman/listinfo/keycloak-dev
> > > > > > > > --
> > > > > > > >
> > > > > > > > abstractj
> > > > > > --
> > > > > >
> > > > > > abstractj
> > > > > > _______________________________________________
> > > > > > keycloak-dev mailing list
> > > > > > keycloak-dev at lists.jboss.org
> > > > > > https://lists.jboss.org/mailman/listinfo/keycloak-dev
> > > > > >
> > > > >
> > > --
> > >
> > > abstractj
> >
>
> --
>
> abstractj
>