Consider relaxing the version check and making it more consistent

[G-Rath]

It's great to see that we've got ensure_consistent_versioning but I feel like it's missing the mark a bit and has inconsistent behaviour.

Sample of current output for reference:

**ERROR** Webpacker: Your node package version for shakapacker contains a ^ or ~
Detected: ^6.4.1
     gem: 6.4.1
Ensure the installed version of the gem is the same as the version of
your installed node package. Do not use >= or ~> in your Gemfile for shakapacker.
Do not use ^ or ~ in your package.json for shakapacker.
Run `yarn add shakapacker --exact` in the directory containing folder node_modules.

1. it does not enforce the use of an exact version in the Gemfile, but does in package.json
2. it's message says "Do not use >= or ~> in your Gemfile for shakapacker.", which imo is silly because you definitely should be using at least a major version constraint (e.g. ~> 6.0)
   • if 1. was fixed the message would become more accurate, but if 1. is fixed by enforcing an exact version then the message should say "any constraint" rather than just two types of constraints
3. the fact that we have ensure_consistent_versioning should mean we can use non-exact versions since we'll be told if the gem and npm package do not match

It's really that last point that I want to talk about: generally it's considered bad practice to depend on an exact version as it makes it harder to do upgrades (especially for security patches) and it's generally a smell of a brittle setup if patch versions (which are meant to be the most non-breaking of changes) matter.

I totally get why it's important to ensure the gem and package are on the same version for this library and that's why I think having ensure_consistent_versioning is the way to go because it should actually allow us to avoid needing to use exact constraints as if something results in the gem being updated, we'll get told we have to update the package, and vice versa.

[tomdracz]

Thanks @G-Rath Good thoughts!

We don't look in the Gemfile as we're getting current version from gem itself. Agree that this makes messaging bit confusing. We do try and guide people to exact version in the installation instructions and template scripts (bundle add shakapacker --strict suggested and --exact option passed in the template installer)

Digesting what you're asking for/suggesting - am I right thinking that ideal setup gets rid of the exact/non-exact versions checks and just shouts at you if the installed combination ends up with two different versions?

This would be pretty neat but it does create some issues. We can be pretty certain about gem version - we're doing check in the gem itself, it knows what it is! npm package however - that's bit more tricky. package.json will only tell you the range. Stopping the non-exact versions from being used is less than elegant, but working solution - it allows for the clean comparison.

The more elegant version needs more information. Either some parsing of yarn.lock or using something like yarn info to extract the package info. If we can know what package version exactly is installed, then we could do away with the wildcard checks.

Don't have a solution on hand but will ponder this, suggestions and PRs also welcome!

[justin808]

Parsing the Gemfile.lock and yarn.lock could be better.

PRs welcome!

[G-Rath]

am I right thinking that ideal setup gets rid of the exact/non-exact versions checks and just shouts at you if the installed combination ends up with two different versions?

Yes that is what I think would be the ideal.

npm package however - that's bit more tricky. package.json will only tell you the range. Stopping the non-exact versions from being used is less than elegant, but working solution - it allows for the clean comparison.

🤦 yeah for some reason I thought you had the exact version of the npm package already and that was just silly.

Parsing the Gemfile.lock and yarn.lock could be better.

Luckily I've actually created stable lockfile parsers for Gemfile.lock, package-lock.json*, and yarn.lock for another project of mine - I should be able to rewrite these in Ruby for this project if you want. (for Gemfile.lock though I'm pretty sure we should just be able to ask bundler directly to tell us the version - my parser is actually just a reimplementation of their parser in Go)

*: I'd like to include support for package-lock.json as I think we're getting closer and closer to folks being able to choose to use npm over yarn for their Rails projects without running into a bunch of stuff that needs to be hacked around and package-lock.json is JSON so it's pretty easy to support - it's just a matter of knowing the structure so you can find the version of the package you're interested in, which I do know :)

[G-Rath]

here you go

# @param [String] line
# @return [Boolean]
def should_skip(line)
  line.empty? || line.start_with?("#")
end

# @param [String] path_to_yarn_lock
# @return [Array<Array<String>>]
def read_yarn_lock(path_to_yarn_lock)
  groups = []
  group = []

  File.foreach(path_to_yarn_lock) do |line|
    # remove the newline at the end of the line
    line = line.delete_suffix("\n")

    next if should_skip(line)

    # represents the start of a new dependency
    unless line.start_with?(" ")
      groups << group unless group.empty?
      group = []
    end

    group << line
  end

  groups << group unless group.empty?
  groups
end

# @param [String] line
# @return [String]
def extract_package_name(line)
  line = line.delete_prefix("\"")
  is_scoped = line.start_with?("@")

  line = line.delete_prefix("@") if is_scoped

  name = line.split("@", 2)[0]

  name = "@#{name}" if is_scoped
  name
end

# @param [Array<String>] group
# @return [String]
def determine_package_version(group)
  group.each do |line|
    m = line.match(/\A {2}version:? "?(?<package_version>[\w.-]+)"?\z/)

    next unless m

    v = m[:package_version]

    return v if v
  end
  "<unknown>"
end

# @param [Array<String>] group
# @return [Array(String, String)]
def parse_package_group(group)
  [extract_package_name(group[0]), determine_package_version(group)]
end

def parse_yarn_lock(path_to_yarn_lock)
  groups = read_yarn_lock(path_to_yarn_lock)

  groups.filter_map do |group|
    # skip yarn v2's metadata property
    next nil if group[0] == "__metadata:"

    parse_package_group(group)
  end
end

ARGV.each do |path_to_yarn_lock|
  puts "#{path_to_yarn_lock}:"
  parse_yarn_lock(path_to_yarn_lock).each do |pkg|
    puts "  #{pkg[0]}@#{pkg[1]}"
  end
end

output

osv-detector on  adjust-exit-code [$!?] via 🐹 v1.17.9 via 💎 v3.1.2
❯ ruby yarn-lock-parser.rb pkg/lockfile/fixtures/yarn/*
pkg/lockfile/fixtures/yarn/commits.v1.lock:
  mine1@1.0.0-alpha.37
  mine2@0.0.1
  mine3@1.2.3
  mine4@0.0.2
  mine4@0.0.4
  my-package@1.8.3
  @bower_components/angular-animate@1.4.14
  @bower_components/alertify@0.0.0
  minimist@0.0.8
  bats-assert@2.0.0
  bats-support@0.3.0
  bats@1.5.0
  vue@2.6.12
  kit@1.0.0
  casadistance@1.0.0
  babel-preset-php@1.1.1
  is-number@2.0.0
  is-number@5.0.0
pkg/lockfile/fixtures/yarn/commits.v2.lock:
  @my-scope/my-first-package@0.0.6
  my-second-package@0.2.2
  @typegoose/typegoose@7.2.0
  vuejs@2.5.0
  my-third-package@0.16.1-dev
  my-node-sdk@1.1.0
  is-really-great@1.0.0
pkg/lockfile/fixtures/yarn/empty.v1.lock:
pkg/lockfile/fixtures/yarn/empty.v2.lock:
pkg/lockfile/fixtures/yarn/files.v1.lock:
  etag@1.8.1
  filedep@1.2.0
  lodash@1.3.1
  other_package@0.0.2
  sprintf-js@0.0.0
  etag@1.8.0
pkg/lockfile/fixtures/yarn/files.v2.lock:
  my-package@0.0.2
pkg/lockfile/fixtures/yarn/metadata-only.v2.lock:
pkg/lockfile/fixtures/yarn/multiple-constraints.v1.lock:
  domelementtype@1.3.1
  @babel/code-frame@7.12.13
pkg/lockfile/fixtures/yarn/multiple-versions.v1.lock:
  define-properties@1.1.3
  define-property@0.2.5
  define-property@1.0.0
  define-property@2.0.2
pkg/lockfile/fixtures/yarn/multiple-versions.v2.lock:
  debug@4.3.3
  debug@2.6.9
  debug@3.2.7
pkg/lockfile/fixtures/yarn/one-package.v1.lock:
  balanced-match@1.0.2
pkg/lockfile/fixtures/yarn/one-package.v2.lock:
  balanced-match@1.0.2
pkg/lockfile/fixtures/yarn/scoped-packages.v1.lock:
  @babel/code-frame@7.12.11
  @babel/compat-data@7.14.0
pkg/lockfile/fixtures/yarn/scoped-packages.v2.lock:
  @babel/cli@7.16.8
  @babel/code-frame@7.16.7
  @babel/compat-data@7.16.8
pkg/lockfile/fixtures/yarn/two-packages.v1.lock:
  concat-map@0.0.1
  concat-stream@1.6.2
pkg/lockfile/fixtures/yarn/two-packages.v2.lock:
  compare-func@2.0.0
  concat-map@0.0.1
pkg/lockfile/fixtures/yarn/versions-with-build-strings.v1.lock:
  css-tree@1.0.0-alpha.37
  gensync@1.0.0-beta.2
  node-fetch@3.0.0-beta.9
  resolve@1.20.0
  resolve@2.0.0-next.3
pkg/lockfile/fixtures/yarn/versions-with-build-strings.v2.lock:
  @nicolo-ribaudo/chokidar-2@2.1.8-no-fsevents.3
  gensync@1.0.0-beta.2
  eslint-plugin-jest@0.0.0-use.local

[justin808]

@G-Rath Please submit a PR.

[G-Rath]

@justin808 yup will do when I've got the time - just wanted to put that here in the meantime incase anyone else had comments or time to take it further :)

Hopefully I'll have some time for a PR on Friday.

[tomdracz]

Nice one @G-Rath ! Looking good on the first glance, will just need to make sure we can support both yarn v1 and berry lockfiles. And yeah, getting support for non-yarn package managers (or well, at least npm), could be useful. Apart from installation and hardcoded usage, there should be no reason (famous last words), why NPM wouldn't work

[G-Rath]

@tomdracz yeah that parser is a direct reimplementation of osv-detectors, so it supports both both v1 and v2 lockfiles