Issue 5856 - Pass --no-package-lock argument to npm install

Issue 5856 - Pass --no-package-lock argument to npm install

[kzar, 2017-10-12 11:51:08]

Patch Set 1

https://codereview.adblockplus.org/29574577/diff/29574578/.hgignore
File .hgignore (left):

https://codereview.adblockplus.org/29574577/diff/29574578/.hgignore#oldcode5
.hgignore:5: .tox
I noticed this entry wasn't consistent with the .gitignore and thought I might
as well fix it here.

[tlucas, 2017-10-12 12:01:59]

https://codereview.adblockplus.org/29574577/diff/29574578/.gitignore
File .gitignore (right):

https://codereview.adblockplus.org/29574577/diff/29574578/.gitignore#newcode4
.gitignore:4: /package-lock.json
I don't think this should be necessary - after all, we won't create any
package-lock.json in the future...

For the time being, since there are now untracked package-lock.json files due to
https://hg.adblockplus.org/buildtools/rev/e9059b211107 , i'd rather delete them
manually instead of ignoring them (Maybe with an integration note in #5856)

IIRC, this only happend in adblockpluschrome and adblockplusui - so one would
only have to manually delete the untracked files from

./adblockplusui/buildtools
./buildtools

(No guarantee here though)


... what do you think?

[tlucas, 2017-10-12 12:28:13]

https://codereview.adblockplus.org/29574577/diff/29574578/.gitignore
File .gitignore (right):

https://codereview.adblockplus.org/29574577/diff/29574578/.gitignore#newcode4
.gitignore:4: /package-lock.json
On 2017/10/12 12:01:59, tlucas wrote:
> I don't think this should be necessary - after all, we won't create any
> package-lock.json in the future...
> 
> For the time being, since there are now untracked package-lock.json files due
to
> https://hg.adblockplus.org/buildtools/rev/e9059b211107 , i'd rather delete
them
> manually instead of ignoring them (Maybe with an integration note in #5856)
> 
> IIRC, this only happend in adblockpluschrome and adblockplusui - so one would
> only have to manually delete the untracked files from
> 
> ./adblockplusui/buildtools
> ./buildtools
> 
> (No guarantee here though)
> 
> 
> ... what do you think?

I just noticed your explanation in the ticket, i.e. why you wanted to add this.
IMHO it should never be necessary to manually run 'npm install' (or at least we
should make sure that there is no necessity to ever do so).

On the other hand, if the user runs 'npm install' and creates a
package-lock.json, the notification about a new unstaged file could possibly
prevent an existing package-lock.json to somehow (not sure in what way though,
but i got somehow cautious about this yesterday) mess up an npm installation. If
neither git, nor hg warned about this new file, one could easily overlook
this...

[kzar, 2017-10-12 13:20:12]

Patch Set 2 : Don't ignore the package-lock.json file

https://codereview.adblockplus.org/29574577/diff/29574578/.gitignore
File .gitignore (right):

https://codereview.adblockplus.org/29574577/diff/29574578/.gitignore#newcode4
.gitignore:4: /package-lock.json
On 2017/10/12 12:28:13, tlucas wrote:
> On 2017/10/12 12:01:59, tlucas wrote:
> > I don't think this should be necessary - after all, we won't create any
> > package-lock.json in the future...
> > 
> > For the time being, since there are now untracked package-lock.json files
due
> to
> > https://hg.adblockplus.org/buildtools/rev/e9059b211107 , i'd rather delete
> them
> > manually instead of ignoring them (Maybe with an integration note in #5856)
> > 
> > IIRC, this only happend in adblockpluschrome and adblockplusui - so one
would
> > only have to manually delete the untracked files from
> > 
> > ./adblockplusui/buildtools
> > ./buildtools
> > 
> > (No guarantee here though)
> > 
> > 
> > ... what do you think?
> 
> I just noticed your explanation in the ticket, i.e. why you wanted to add
this.
> IMHO it should never be necessary to manually run 'npm install' (or at least
we
> should make sure that there is no necessity to ever do so).
> 
> On the other hand, if the user runs 'npm install' and creates a
> package-lock.json, the notification about a new unstaged file could possibly
> prevent an existing package-lock.json to somehow (not sure in what way though,
> but i got somehow cautious about this yesterday) mess up an npm installation.
If
> neither git, nor hg warned about this new file, one could easily overlook
> this...

Yea good point, I think I agree. Done.

[tlucas, 2017-10-12 13:21:20]

let's see what the other think :)

LGTM though

[Sebastian Noack, 2017-10-12 14:53:40]

LGTM!