[keycloak-dev] Merging node_modules into Keycloak repo

[Stan Silvert]

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.
> 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
> 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)?
> 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.
> 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?

How is Aerogear dealing with productization?
>
>
>> Any thoughts?
>> _______________________________________________
>> keycloak-dev mailing list
>> keycloak-dev at lists.jboss.org
>> https://lists.jboss.org/mailman/listinfo/keycloak-dev
> --
>
> abstractj