Add support for npm 7 lockfiles by feelepxyz · Pull Request #3011 · dependabot/dependabot-core

feelepxyz · 2021-01-19T14:25:36Z

Add support for updating npm 7 lockfiles alongside npm 6 based on the lockfileVersion set in the package-lock.json file. If this is set to 2 we used npm 7 otherwise fallback to npm 6.

The native helpers to update dependencies shell out to npm install pkg-name for direct/top-level and npm update pkg-name for indirect/sub-dependencies.

We made an initial attempt to use arborist programatically to do the update but abandoned this approach as we ran into a bug where arborist depends on pacote which re-runs what it thinks is the main npm cli command on failures to retry, which in our setup lead to an infinite loop of retries. The bug was reproduced here: https://github.com/feelepxyz/npm7-hang-investigation and discussed with the npm team who advised us to no use arborist programatically in this way (yet).

Review checklist/notable changes

Rebase on top of Extract npm 7 project fixtures and update specs #3086
Review relevant files: https://github.com/dependabot/dependabot-core/pull/3011/files?file-filters%5B%5D=.js&file-filters%5B%5D=.rb&file-filters%5B%5D=dotfile
Changes to call out
- npm_and_yarn/lib/dependabot/npm_and_yarn/dependency_files_filterer.rb
  - Changes required to support npm 7 workspaces
- npm_and_yarn/lib/dependabot/npm_and_yarn/file_updater/npm_lockfile_updater.rb
  - Handle new error output
    - npm 7 now exits with a full error log so we can't do starts_with? on anything meaningful
  - handle updating the root lockfile when using workspaces in top_level_dependency_update_not_required?
  - Handle locked git dependencies, when updating pkg-a we go through and find any top-level git dependencies and lock them to the current resolved sha in package.json and then re-set the lockfile after update
  - Handle npm 7 lockfile packages."".*dependencies entries in restore_locked_package_dependencies which should mirror what's in your package.json, we need to reset this after update otherwise we'll have both locked git deps and updated dependencies new requirements in there even though we didn't update the package.json
    - If your package.json says eslint: ^1.0.0 but we want to update to 1.1.1 without updating the package.json we run npm install eslint@1.1.0 which updates the packages."".*dependencies entry to 1.1.0 but we want it to match the package.json so reset it to ^1.0.0 after update
- git_dependencies_to_lock git dependencies from syntax in lockfiles has changed to include the dependency name and also always seem to default to ssh:// protocol when not explicitly fetching from https://github.. for example
  - It seems like a new default as it changes when converting a npm6 lockfile to npm 7 with npm i --package-lock-only

Release plan

Review ⏫
Dry run existing npm 6 updates
Dry run npm 7 projects from around github, how to search for "\"lockfileVersion\": 2" with quotes?

This adds a couple launch scripts to debug npm and yarn tests when running then using jest. The launch scripts complained about a missing config file so added this and changed the default env from jsdom to node as this is what we use. Changed `eslint-plugin-prettier` to `eslint-config-prettier` as this seems like the recommended extension now when using prettier with eslint: https://prettier.io/docs/en/integrating-with-linters.html This extracts these changes from the npm 7 branch: #3011

Extract all project based spec fixtures from npm7 pr to reduce diff: #3011 The file updater specs changes are copied but excluding the npm 7 specifc specs.

feelepxyz · 2021-02-05T15:08:51Z

          # consistency issues and the version isn't fully available on all
          # queries
-          if error_message.start_with?("No matching vers") &&
+          if error_message.include?("No matching vers") &&


Why don't we match on the full string here? No matching version

jurre · 2021-02-05T15:28:56Z

-        def post_process_npm_lockfile(original_content, updated_content)
-          updated_content =
-            replace_project_metadata(updated_content, original_content)
+        def post_process_npm_lockfile(original_content, updated_content, lockfile_name)


As discussed in pair-review: this might be a good candidate to extract out into its own class

Yeah agreed, there's a bunch of pre/post-processing that's intertwined and would probably be a lot easier to grok if it was in one place.

feelepxyz · 2021-02-08T12:00:13Z

      end
-    end

-    context "when a git src dependency doesn't have a valid package.json" do


This was only removed from npm 7 as it takes absolutely ages and actually manages to do the update, the spec exists here for npm 6:

dependabot-core/npm_and_yarn/spec/dependabot/npm_and_yarn/file_updater/npm_lockfile_updater_spec.rb

Line 79 in bfbc137

context "when a git src dependency doesn't have a valid package.json" do

feelepxyz · 2021-02-08T12:00:46Z

+          expect(parsed_lockfile["dependencies"]["fetch-factory"]["version"]).
+            to eq("0.0.2")

-    context "with a corrupted npm lockfile (version missing)" do


No longer failing in npm 7, npm 6 spec is here

dependabot-core/npm_and_yarn/spec/dependabot/npm_and_yarn/file_updater/npm_lockfile_updater_spec.rb

Line 64 in bfbc137

context "with a corrupted npm lockfile (version missing)" do

feelepxyz · 2021-02-08T12:01:28Z

-    end

+    # NOTE: This no longer raises in npm 7
    context "when there is a private git dep we don't have access to" do


No longer fails for npm 7, tests have moved here:

dependabot-core/npm_and_yarn/spec/dependabot/npm_and_yarn/file_updater/npm_lockfile_updater_spec.rb

Lines 131 to 170 in bfbc137

# NOTE: This no longer raises in npm 7

context "when there is a private git dep we don't have access to" do

let(:files) { project_dependency_files("npm6/github_dependency_private") }

let(:dependency_name) { "strict-uri-encode" }

let(:version) { "1.1.0" }

let(:requirements) { [] }

it "raises a helpful error" do

expect { updated_npm_lock_content }.

to raise_error(Dependabot::GitDependenciesNotReachable) do |error|

expect(error.dependency_urls).

to eq(

[

"https://github.com/hmarr/dependabot-test-private-npm-package.git/"

]

)

end

end

end

end

describe "npm 7 specific" do

# NOTE: This used to raise in npm 6

context "when there is a private git dep we don't have access to" do

let(:files) { project_dependency_files("npm7/github_dependency_private") }

let(:dependency_name) { "strict-uri-encode" }

let(:version) { "1.1.0" }

let(:requirements) { [] }

it "updates the dependency and leaves the private git dep alone" do

parsed_lockfile = JSON.parse(updated_npm_lock_content)

expect(parsed_lockfile.fetch("dependencies")["strict-uri-encode"]["version"]).

to eq("1.1.0")

expect(parsed_lockfile.fetch("dependencies")["bus-replacement-service"]["version"]).

to include("19c4dba3bfce7574e28f1df2138d47ab4cc665b3")

end

end

end

feelepxyz · 2021-02-08T12:16:40Z

+            "--ignore-scripts",
+            "--package-lock-only"
+          ].join(" ")
+          SharedHelpers.run_shell_command(command)


Note: command gets escaped by shellwords https://github.com/dependabot/dependabot-core/blob/main/common/lib/dependabot/shared_helpers.rb#L270

feelepxyz · 2021-02-08T12:17:07Z

          sub_dep_local_path_error = "does not contain a package.json file"
          if error_message.match?(INVALID_PACKAGE) ||
-             error_message.start_with?("Invalid package name") ||
+             error_message.include?("Invalid package name") ||


Note: npm 7 now exits with a full error log so we can't do starts_with? on anything meaningful

jurre · 2021-02-08T12:28:23Z

+          end
+        end
+
+        def run_npm_7_subdependency_updater(lockfile_name:)


Mentioned while pairing: might want to extract a separate Npm7LockfileUpdater in a follow-up PR.

jurre · 2021-02-08T13:49:13Z

+        # need to copy this from the manifest to the lockfile after the update
+        # has finished.
+        def restore_locked_package_dependencies(lockfile_name, lockfile_content)
+          npm_version = Dependabot::NpmAndYarn::Helpers.npm_version(lockfile_content)


We check for the npm_version 6 times in this file, I know we talked about extracting an npm 7 updater, but maybe we can start with:

def npm7? return @npm7 if defined?(@npm7) @npm7 = Dependabot::NpmAndYarn::Helpers.npm_version(lockfile_content) end

And using that everywhere? Might be a micro-optimization but I think it slightly clarifies the code and should be a pretty cheap change?

Yeah this would be good but I think we need to refactor the lockfile updater a bit to get there as we currently pass around the lockfile name everywhere from here:

dependabot-core/npm_and_yarn/lib/dependabot/npm_and_yarn/file_updater/npm_lockfile_updater.rb

Line 26 in 720e663

def updated_lockfile_content(lockfile)

Initialising the class with a single lockfile to be updated would make the class a lot simpler.

Extracted it into a method but still passing around the content

jurre

I think we should keep iterating on the code, but functionally I feel pretty confident about this

feelepxyz · 2021-02-08T14:21:59Z

Investigating python failures on main, seems unrelated to any recent changes

feelepxyz requested a review from a team as a code owner January 19, 2021 14:25

feelepxyz marked this pull request as draft January 19, 2021 14:26

feelepxyz mentioned this pull request Jan 19, 2021

Fix JS debugging in vscode #3016

Merged

jurre force-pushed the feelepxyz/use-npm7 branch 2 times, most recently from 01ed498 to 144e747 Compare January 26, 2021 12:17

feelepxyz force-pushed the feelepxyz/use-npm7 branch from 7998d92 to 759836f Compare February 2, 2021 09:44

feelepxyz mentioned this pull request Feb 3, 2021

Refactor npm 6 specs #3072

Merged

feelepxyz force-pushed the feelepxyz/use-npm7 branch from 5017ca7 to 0dc71c8 Compare February 4, 2021 13:57

feelepxyz commented Feb 4, 2021

View reviewed changes

Comment thread npm_and_yarn/lib/dependabot/npm_and_yarn/file_updater/npm_lockfile_updater.rb Outdated

jurre mentioned this pull request Feb 4, 2021

package-lock.json downgrades from v2 (newly released npm@7) to v1 #2643

Closed

feelepxyz and others added 19 commits February 5, 2021 11:14

Add npm7 helpers (using npm6)

3bc2310

WIP: npm7:update script

4112870

Add npm7 js tests

ec04bd9

Fix vscode debugging

269724a

Add npm7 file updater spec

32b7460

Share conflicting dependency parser between npm 6 and 7

e145400

Fix conflicting dependency checker

0de3c29

Add jest config for debugging

6b7c875

Update peer dependency checker for npm 7

eb1fd16

set jest config

a987c03

Fix jest config

2acf9a6

Fix bad merge

dde264b

Add npm 7 version resolver specs

f9b1e6c

Update npm 7 file updater spec

0acc128

Update subdependencies using npm7

ceb5048

Add npm file updater error spec

9212623

Test NpmLockfileUpdater with npm7

b22b1be

Ensure invalid refs return a sensible error in npm7

bc2e1cf

Use npm error message util to format errors

27cab22

feelepxyz added 2 commits February 5, 2021 11:15

Copy npm 6 specs to npm 7

0e52ae8

npm 7: package-lock-only updates ✨

3bd9de4

feelepxyz changed the title ~~WIP: Add support for npm 7 lockfiles~~ Add support for npm 7 lockfiles Feb 5, 2021

feelepxyz marked this pull request as ready for review February 5, 2021 11:25

feelepxyz force-pushed the feelepxyz/use-npm7 branch from 15ffd14 to 3bd9de4 Compare February 5, 2021 13:59

feelepxyz added a commit that referenced this pull request Feb 5, 2021

Extract npm 7 project fixtures and update specs

e5faa79

Extract all project based spec fixtures from npm7 pr to reduce diff: #3011 The file updater specs changes are copied but excluding the npm 7 specifc specs.

feelepxyz mentioned this pull request Feb 5, 2021

Extract npm 7 project fixtures and update specs #3086

Merged

Merge remote-tracking branch 'origin/main' into feelepxyz/use-npm7

7bea0c1

feelepxyz commented Feb 5, 2021

View reviewed changes

Comment thread npm_and_yarn/helpers/lib/npm7/updater.js Outdated

feelepxyz commented Feb 5, 2021

View reviewed changes

jurre reviewed Feb 5, 2021

View reviewed changes

feelepxyz added 2 commits February 8, 2021 11:10

Use shared helper to run npm 7 updates

94daecf

Extract npm 7 updaters to their own methods

bfbc137

feelepxyz commented Feb 8, 2021

View reviewed changes

Improve comments for npm lockfile updater

80c9954

feelepxyz commented Feb 8, 2021

View reviewed changes

Comment thread npm_and_yarn/lib/dependabot/npm_and_yarn/dependency_files_filterer.rb

feelepxyz commented Feb 8, 2021

View reviewed changes

Merge remote-tracking branch 'origin/main' into feelepxyz/use-npm7

b74b038

jurre reviewed Feb 8, 2021

View reviewed changes

Comment thread npm_and_yarn/lib/dependabot/npm_and_yarn/file_updater/npm_lockfile_updater.rb Outdated

jurre reviewed Feb 8, 2021

View reviewed changes

Use memoized updated package json content for npm 7 install args

720e663

jurre reviewed Feb 8, 2021

View reviewed changes

jurre approved these changes Feb 8, 2021

View reviewed changes

Extract npm 7 check to method

7971b82

feelepxyz merged commit 19fef63 into main Feb 8, 2021

feelepxyz deleted the feelepxyz/use-npm7 branch February 8, 2021 14:43

	# NOTE: This no longer raises in npm 7
	context "when there is a private git dep we don't have access to" do
	let(:files) { project_dependency_files("npm6/github_dependency_private") }

	let(:dependency_name) { "strict-uri-encode" }
	let(:version) { "1.1.0" }
	let(:requirements) { [] }

	it "raises a helpful error" do
	expect { updated_npm_lock_content }.
	to raise_error(Dependabot::GitDependenciesNotReachable) do \|error\|
	expect(error.dependency_urls).
	to eq(
	[
	"https://github.com/hmarr/dependabot-test-private-npm-package.git/"
	]
	)
	end
	end
	end
	end

	describe "npm 7 specific" do
	# NOTE: This used to raise in npm 6
	context "when there is a private git dep we don't have access to" do
	let(:files) { project_dependency_files("npm7/github_dependency_private") }

	let(:dependency_name) { "strict-uri-encode" }
	let(:version) { "1.1.0" }
	let(:requirements) { [] }

	it "updates the dependency and leaves the private git dep alone" do
	parsed_lockfile = JSON.parse(updated_npm_lock_content)
	expect(parsed_lockfile.fetch("dependencies")["strict-uri-encode"]["version"]).
	to eq("1.1.0")
	expect(parsed_lockfile.fetch("dependencies")["bus-replacement-service"]["version"]).
	to include("19c4dba3bfce7574e28f1df2138d47ab4cc665b3")
	end
	end
	end

Conversation

feelepxyz commented Jan 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review checklist/notable changes

Release plan

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

feelepxyz Feb 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jurre left a comment

Choose a reason for hiding this comment

Uh oh!

feelepxyz commented Feb 8, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

feelepxyz commented Jan 19, 2021 •

edited

Loading

feelepxyz Feb 8, 2021 •

edited

Loading