Problem/Motivation

With NPM version 5 a package-lock.json file is created automatically when adding dependencies. Since we are using Yarn to manage Node.js dependencies, we want to avoid an accidental inclusion of a package-lock.json in patches by developers that have accidently used NPM.

Proposed resolution

Add package-lock.json to .gitignore.

CommentFileSizeAuthor
#17 2881697-17.patch373 bytesGrandmaGlassesRopeMan
#17 interdiff-2881697-0-17.txt243 bytesGrandmaGlassesRopeMan
#2 ignore-package-lock.patch266 bytesdroplet
ignore-package-lock.patch267 bytesdroplet
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

droplet created an issue. See original summary.

droplet’s picture

droplet CreditAttribution: droplet commented
FileSize
ignore-package-lock.patch266 bytes
nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ as a volunteer commented

should we switch back to npm5 once it's out instead of yarn? lots of improvments

droplet’s picture

droplet CreditAttribution: droplet commented

Still don't know how well the package-lock.json is.

Yarn still faster than npm@5 on my testing.

With warm caching and installed packages (node_modules). Yarn only took 1 sec to re-check the dependencies but npm@5 took 6 sec (Including command exit time)

droplet’s picture

droplet CreditAttribution: droplet commented

Yarn maintainers give us some direction. Although, that's not much. @see: https://yarnpkg.com/blog/2017/05/31/determinism/

GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan
he/him
Primary language English
Location 🌎
CreditAttribution: GrandmaGlassesRopeMan at Acquia commented

Yarn still appears to be faster in the testing I've done. I don't really have a preference, but we have written all the docs pointing to yarn.

droplet’s picture

droplet CreditAttribution: droplet commented

Yeah, I think we can commit this patch first. v8 just released but still a few months away from the LTS version.

nodejs <-> io.js
npm <-> Yarn

Drupal is quite similar to the older NODEJS / NPM development style. Moving very slowly (and they think that's the best development way already). No idea which new competitor could give Drupal some pressure, haha!

GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan
he/him
Primary language English
Location 🌎
CreditAttribution: GrandmaGlassesRopeMan at Acquia commented
Status: Active » Reviewed & tested by the community

@droplet

Yeah. I think it's this fall, somewhere in the 8.x release cycle. Otherwise, 👍

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Druid commented
Status: Reviewed & tested by the community » Needs review

Doesn't this only happen if you use npm install? Why should we make this change if we are currently trying to make people using yarn for installing packages?

droplet’s picture

droplet CreditAttribution: droplet commented
Status: Needs review » Reviewed & tested by the community

@lauriii,

Good Question!

I think for some reason, developers may run `npm install developer-helper`. Actually, some packages will fail to install on Yarn. Or say if you're a PHP developer, run Yarn or NPM won't affect the patching :)

cilefen’s picture

cilefen CreditAttribution: cilefen commented
Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs issue summary update

Could someone please explain in a few more words in the issue summary why package-lock.json is a bad thing?

GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan
he/him
Primary language English
Location 🌎
CreditAttribution: GrandmaGlassesRopeMan at Acquia commented
Issue summary: View changes
Issue tags: -Needs issue summary update
cilefen’s picture

cilefen CreditAttribution: cilefen commented

Ah!

cilefen’s picture

cilefen CreditAttribution: cilefen commented
Status: Needs review » Reviewed & tested by the community
GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan
he/him
Primary language English
Location 🌎
CreditAttribution: GrandmaGlassesRopeMan at Acquia commented

👏 👍

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii as a volunteer commented
Status: Reviewed & tested by the community » Needs work

Sorry, but I think we should also include this explanation in the .gitignore like we do have for the other lines as well.

GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan
he/him
Primary language English
Location 🌎
CreditAttribution: GrandmaGlassesRopeMan at Acquia commented
Assigned: Unassigned » lauriii
Status: Needs work » Needs review
FileSize
interdiff-2881697-0-17.txt243 bytes
2881697-17.patch373 bytes

I provided a slightly shorter description for .gitignore.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii as a volunteer commented
Assigned: lauriii » Unassigned
Status: Needs review » Reviewed & tested by the community

This one looks good for me.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick at Acquia commented
Status: Reviewed & tested by the community » Fixed

Ok, looks like this one is straight-forward and has approval from the right folks.

Committed and pushed to 8.4.x. Thanks!

  • webchick committed 481b588 on 8.4.x 
    Issue #2881697 by droplet, drpal, cilefen, lauriii: npm@5 creates...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.