Skip to content

Commit

Permalink
Instrument pnpm version
Browse files Browse the repository at this point in the history
  • Loading branch information
@deivid-rodriguez
deivid-rodriguez committed Apr 26, 2023
1 parent 103a255 commit c1712c9
Show file tree
Hide file tree
Showing 2 changed files with 22 additions and 0 deletions.
13 changes: 13 additions & 0 deletions npm_and_yarn/lib/dependabot/npm_and_yarn/file_fetcher.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ def package_manager_version

package_managers["npm"] = Helpers.npm_version_numeric(package_lock.content) if package_lock
package_managers["yarn"] = yarn_version if yarn_version
package_managers["pnpm"] = pnpm_version if pnpm_version
package_managers["shrinkwrap"] = 1 if shrinkwrap
package_managers["unknown"] = 1 if package_managers.empty?

Expand Down Expand Up @@ -169,6 +170,18 @@ def guess_yarn_version
Helpers.yarn_version_numeric(yarn_lock)
end

def pnpm_version
return @pnpm_version if defined?(@pnpm_version)

@pnpm_version = package_manager.locked_version("pnpm") || guess_pnpm_version
end

def guess_pnpm_version
return unless pnpm_lock

This comment has been minimized.

Sign in to view

Copy link
@jeffwidman

jeffwidman Apr 27, 2023

Member

🤔 If there is no pnpm_lock, I assume this returns nil all the way back up the stack... Will this cause problems?

IIRC, the only way we even know to use pnpm is due to the presence of the pnpm_lock file, so today we should never hit this case.

But if we later change things so that users tell us explicitly: "use pnpm" rather than "use npm_and_yarn" to use pnpm, then this could start happening where we get nil rather than a version.

In that case, will the absence of a lockfile mean that the pnpm version simply doesn't matter? Will it still update package.json, just using pnpm instead of npm??

To be clear, this isn't a big deal today, just trying to think ahead defensively... wondering if we should always default guessing to returning the pnpm major version, regardless of presence of the lockfile... 🤷‍♂️

This comment has been minimized.

Sign in to view

Copy link
@deivid-rodriguez

deivid-rodriguez Apr 28, 2023

Author Contributor

Good point!

First thing to note is that, as of today, this code is only used for instrumentation purposes. So, yes, as you point out, if there's no lockfile, we can't really know the package manager being used here. As a result, we return nil. That ultimately means that we're probably going to instrument the "NPM" package manager as "unknown", which seems right (see first block of code changed in this commit).

As per what happens in "no lockfile" situations, I think we end up not really using any package manager, just update the package.json manually.

For now this seems all expected and it's exactly what we already do for Yarn, but stuff to keep on mind for #7189 and related issues (great idea, by the way!).

Helpers.pnpm_major_version
end

def package_manager
@package_manager ||= PackageManager.new(parsed_package_json)
end
Expand Down
9 changes: 9 additions & 0 deletions npm_and_yarn/lib/dependabot/npm_and_yarn/helpers.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,11 +43,20 @@ def self.yarn_major_version
@yarn_major_version ||= fetch_yarn_major_version
end

def self.pnpm_major_version
@pnpm_major_version ||= fetch_pnpm_major_version
end

def self.fetch_yarn_major_version
output = SharedHelpers.run_shell_command("yarn --version")
Version.new(output).major
end

def self.fetch_pnpm_major_version
output = SharedHelpers.run_shell_command("pnpm --version")
Version.new(output).major
end

def self.yarn_zero_install?
File.exist?(".pnp.cjs")
end
Expand Down

0 comments on commit c1712c9

Please sign in to comment.