[keycloak-dev] Merging node_modules into Keycloak repo

Fri May 12 05:00:39 EDT 2017

On 2017-05-12, Stian Thorgersen wrote:
> 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 don't have to do that with `frontend-maven-plugin`. To make sure
that I'm not saying anything stupid, I built admin-ui from the
UnifiedPush server(https://github.com/aerogear/aerogear-unifiedpush-server/tree/master/admin-ui)

mvn clean install - will get all the node_modules that we need.
mvn clean - will keep node_modules

One thing that I noticed now on the same plugin. You don't need to install
node by yourself. The plugin will do that for you. See:
https://github.com/eirslett/frontend-maven-plugin#installing-node-and-yarn

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

--

abstractj