Fix: Correctly parse v3 lockfiles

[cristiano-belloni]

Problem

• @yarnpkg/lockfile is able to parse lockfile v1 format correctly but not lockfile v3 format (it throws immediately)
• The tools based on @yarnpkg/lockfile that we were using error with the lockfile v3 format
• It seems that lockfiles v2+ are not supported because they are valid yaml

Solution

• Try to parse lockfile directly with @yarnpkg/lockfile
• If it crashes, parse it manually from YAML.

[changeset-bot]

🦋 Changeset detected

Latest commit: 35ca64c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
modular-scripts	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[coveralls]

Coverage decreased (-0.1%) to 26.667% when pulling 35ca64c on fix/parse-lockfile-manually into 2c36b08 on main.

[benpryke]

This comment seems to be floating away from what it describes

[cristiano-belloni]

Done

[benpryke]

The getPackageDependencies function is very long. You could begin to break it down by moving each of these v1 and v2+ operations to separate functions.

[steveukx]

I also recommend splitting these into separate functions - it's unclear to the reader which thing might fail and therefore why the catch block is larger than the try block... consider:

function parseYarnLock(lockFile: string, deps: Dependency): Dependency {
  return parseYarnLockV1(lockFile, deps) || parseYarnLockV3(lockFile, deps);
}

function parseYarnLockV1(lockFile: string, deps: Dependency): Dependency | null {
  try {
    const parsedLockfile = lockfile.parse(lockFile);
    return Object.entries(deps).reduce(...);
  }
  catch (e) { return null }
}

function parseYarnLockV3(lockFile: string, deps: Dependency): Dependency {
  return Object.entries(yaml.load(lockFile) as LockFileEntries).reduce(...);
}

These could then be more readily unit/integration tested in isolation.

[cristiano-belloni]

Done

[benpryke]

As you're not actually using the return value of Array.prototype.some, a for loop with a break statement would be clearer.

[cristiano-belloni]

I find it quite idiomatic when we ignore the return value (functional breaking loop), but no prob, done.

[benpryke]

What's the difference between dependencyArray and Object.entries(parsedLockfile)? I don't quite understand what this O(n^2) algo is doing.

[cristiano-belloni]

dependencyArray is the array of dependencies as extracted from source (likely very small), while Object.entries(parsedLockfile) is the array of entries from the lockfile (possibly big). It's quadratic but not as in O(n^2), more like O(n*m), where m << n (greatly less than).

We are traversing all the lockfile entries once and, for every entry, split the key in all the dependencies it represents, then match each with the dependencies extracted from source.

[benpryke]

Thanks for the explanation. Why do we not need to add the dependencies to the right of the parsedLockfile entries after a match is found?

When you extract this to a separate function, I'd recommend adding a little context so it's easier for the next person who reads this to get to grips with the why. I'm not familiar with the new lockfile format!

[cristiano-belloni]

Refactored + added additional comments.

[cangarugula]

Since we are explicitly catering to yarn 1 and yarn 3, can we add this to isYarnInstalled in startupCheck.ts ?

[cangarugula]

By this, I mean the check for the version of yarn to be explicit with our compatibility

[cangarugula]

Spoke offline and decided to create a separate ticket to resolve yarn version compatibility