Re: [keycloak-dev] Merging node_modules into Keycloak repo

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(a)redhat.com&gt; 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? > >>> * 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? 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? > >>> * 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. > > 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(a)abstractj.org&gt; 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(a)lists.jboss.org >>>>>>> https://lists.jboss.org/mailman/listinfo/keycloak-dev >>>>>> -- >>>>>> >>>>>> abstractj >>>> -- >>>> >>>> abstractj >>>> _______________________________________________ >>>> keycloak-dev mailing list >>>> keycloak-dev(a)lists.jboss.org >>>> https://lists.jboss.org/mailman/listinfo/keycloak-dev >>>> >>> > -- > > abstractj _______________________________________________ keycloak-dev mailing list keycloak-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/keycloak-dev