Check in lockfiles

[dyladan]

Creating this draft as a test to see how much faster CI would be with lock files checked in

answer: much much faster. CI is faster using lockfiles than when there is a cache hit

I am creating this PR as a point of discussion. Looking for input from @open-telemetry/javascript-approvers. Do you think we should check in lockfiles?

There is a good summary of the tradeoffs here: https://classic.yarnpkg.com/blog/2016/11/24/lockfiles-for-all/. It is about yarn, but applies to npm as well.

Discussion here: #1830

[codecov]

Codecov Report

Merging #1829 (76fc976) into main (d54c5c8) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1829   +/-   ##
=======================================
  Coverage   92.35%   92.35%           
=======================================
  Files         157      157           
  Lines        5104     5104           
  Branches     1085     1085           
=======================================
  Hits         4714     4714           
  Misses        390      390           

[obecny]

I think we should proceed with this further then.

[dyladan]

ok

[vmarchaud]

💯

[dyladan]

Seems like all maintainers are on board. Marked as ready for reviews and will leave for a day or so to ensure nobody has any complaints before merging.

[Flarna]

Are the PRs from renovate-bot taking care of updating lock files also?

I miss also some hints for devs what they shall do if they add/remove/modify a dependency. I use lock files in some projects where I usually deleted lock file + node_modules folder and then run npm install. But here we have lerna so not sure if this works as intended regarding the linked lerna subprojects.

Finally I think https://github.com/open-telemetry/opentelemetry-js/blob/main/CONTRIBUTING.md#install-dependencies should be changed to npm ci.

[Flarna]

Looks like they need a refresh

[dyladan]

Are the PRs from renovate-bot taking care of updating lock files also?

Yes. Per renovate docs https://docs.renovatebot.com/faq/

I miss also some hints for devs what they shall do if they add/remove/modify a dependency. I use lock files in some projects where I usually deleted lock file + node_modules folder and then run npm install. But here we have lerna so not sure if this works as intended regarding the linked lerna subprojects.

It works the same way, but the lock file is regenerated and possibly different. Dependencies cross-linked by lerna are omitted entirely from the package-lock. When lerna bootstrap is run, the package.json files are copied to a backup location and temporary ones are put in place which only contain external dependencies. Then npm install is run. Then symlinks are created between lerna dependencies. Then the original package.json files are moved back.

Finally I think https://github.com/open-telemetry/opentelemetry-js/blob/main/CONTRIBUTING.md#install-dependencies should be changed to npm ci.

It would be lerna bootstrap --ci, but only if you wanted to get the locked version of the dependencies for some reason. A bootstrap will work just as well locally and the lock file will be updated if it needs to be.

[Flarna]

opentelemetry-plugin-fetch has been removed so we should not add a lockfile