Prioritize direct dependency if available #192

thatsmydoing · 2022-09-09T13:13:25Z

This is more of a proof of concept and request for comments. This PR adds support for prioritizing direct dependencies (ones listed in package.json).

For example, if our package.json has the following

{
  "devDependencies": {
    "@types/react": "^17.0.49",
    "@types/react-redux": "^7.1.24"
  }
}

at the time of writing, this will result in these yarn.lock entries

"@types/react@*":
  version "18.0.18"
  resolved "https://registry.yarnpkg.com/@types/react/-/react-18.0.18.tgz#9f16f33d57bc5d9dca848d12c3572110ff9429ac"
  integrity sha512-6hI08umYs6NaiHFEEGioXnxJ+oEhY3eRz8VCUaudZmGdtvPviCJB8mgaMxaDWAdPSYd4eFavrPk2QIolwbLYrg==
  dependencies:
    "@types/prop-types" "*"
    "@types/scheduler" "*"
    csstype "^3.0.2"

"@types/react@^17.0.49":
  version "17.0.49"
  resolved "https://registry.yarnpkg.com/@types/react/-/react-17.0.49.tgz#df87ba4ca8b7942209c3dc655846724539dc1049"
  integrity sha512-CCBPMZaPhcKkYUTqFs/hOWqKjPxhTEmnZWjlHHgIMop67DsXywf9B5Os9Hz8KSacjNOgIdnZVJamwl232uxoPg==
  dependencies:
    "@types/prop-types" "*"
    "@types/scheduler" "*"
    csstype "^3.0.2"

@types/react-redux pulls in @types/react@* which is correct since it does work with any react version. However, this is problematic for us since when we use the react-redux types, it leaks in the newer react 18 types which are not compatible with the react 17 types. So we absolutely want both to resolve to 17.0.49.

This can be worked around using yarn resolutions which is what we're doing now but it would be nice if yarn-deduplicate could do this for us.

With the default highest strategy, these won't be deduplicated. We could use fewer to deduplicate here but that affects everything else but we'd rather it apply only to direct dependencies like @types/react here.

This PR adds a --package-json flag which you can point to an existing package.json where it can get information on which versions are direct dependencies and then prioritizes those on top of whatever strategy is picked.

Is this sound? I'm not too confident with how this interacts with strategies (and should this be a strategy in itself?)

scinos · 2022-09-24T06:13:10Z

src/cli.ts

@@ -63,10 +65,12 @@ if (strategy !== 'highest' && strategy !== 'fewer') {

 try {
    const yarnLock = fs.readFileSync(file, 'utf8');
+    const packageJson = packageJsonPath ? fs.readFileSync(packageJsonPath, 'utf8') : undefined;


Nit: I rather use null instead of undefined in this case. It feels more explicit to me.

scinos · 2022-09-24T06:17:28Z

tests/index.ts

+      version "0.1.0"
+      resolved "http://example.com/a-package/0.1.0"
+
+    other-package@>=1.0.0:


We should remove any mention to other-package in this test, as it is not related to the test intention.

This is actually important as it verifies that packages that are not in package.json fall back to the highest strategy.

scinos · 2022-09-24T06:19:36Z

tests/index.ts

+
+test('prioritizes direct requirements if present', () => {
+    const yarn_lock = outdent`
+    a-package@*:


Ideally (and I acknowledge this is slightly out of the scope of the PR), we should use a similar yarn.lock to test all strategies (i.e. the first two tests in this file). That way it's more clear what's the different behaviour of each strategy.

scinos · 2022-09-24T06:35:53Z

Interesting idea. I do see the problem you are trying to solve, and this seems like a neat solution.

You mentioned fewer strategy to solve this, but even if we restrict it to @types/react I don't think it will work (or rather, if it works, it's by accident): fewer has no reason to chose 17.0.49 over 18.0.18 (assuming you have only one instance of each, of course).

I think this should be its own strategy, let's call it direct. So the logic is (just putting in words what you already did in code): let's say we have ranges A, B and C overlapping. If A is also defined in package.json, it will dedupe them to A. If A is not defined, it will fallback to highest (I think allowing direct and customize the fallback strategy is too much complexity).

On a higher level, this PR highlight the deeper problem of not having control of which version is used to deduplicate. Using strategies is just a proxy to automatically select the versions. Using direct is another way of saying "use the version I already wrote in that file". Maybe we need something else, something more direct. I'm thinking an interactive strategy which should ask the user for the version (something similar to https://classic.yarnpkg.com/lang/en/docs/cli/upgrade-interactive/). But of course this is way beyond the scope of this PR, it's just a rambling :)

Before merging this PR I'd like to see:

Reframe this as a new strategy
Add docs in README
Address the (very minor) issues in the comments.

I may help with those if you want, but I can't commit to a timeline right now.

scinos · 2022-09-24T06:40:02Z

src/index.ts

@@ -107,9 +137,15 @@ const computePackageInstances = (packages: Packages, name: string, useMostCommon
    // Extract the list of unique versions for this package
    const versions:Versions = new Map();
    for (const packageInstance of packageInstances) {
-        if (versions.has(packageInstance.installedVersion)) continue;
+        if (versions.has(packageInstance.installedVersion)) {
+            const existingPackage = versions.get(packageInstance.installedVersion)!;


I don't get why we need to add isDirectDependency to every dep in the tree.

Wouldn't checking directDependencies.has(entryName) when sorting the versions (line 177) suffice? Especially when we encapsulate this logic into its own strategy.

I've moved this down here so only Version needs the isDirectDependency flag. There's no entryName to check in the version sorting step so it's not that easy to do it there. If we absolutely want to avoid having this, I suppose it's possible to loop over satisfies in the sorting function and check if any requested version is a direct dependency but that'd just be worse for performance.

thatsmydoing

Thanks for the feedback, I'll rework it into a strategy instead.

thatsmydoing · 2022-09-26T03:08:41Z

tests/index.ts

+      version "0.1.0"
+      resolved "http://example.com/a-package/0.1.0"
+
+    other-package@>=1.0.0:


This is actually important as it verifies that packages that are not in package.json fall back to the highest strategy.

thatsmydoing

How about this?

thatsmydoing · 2022-09-26T05:18:07Z

src/cli.ts

@@ -13,7 +13,7 @@ program
    .usage('[options] [yarn.lock path (default: yarn.lock)]')
    .option(
        '-s, --strategy <strategy>',
-        'deduplication strategy. Valid values: fewer, highest. Default is "highest"',
+        'deduplication strategy. Valid values: fewer, highest, direct.',


commander already shows the default value so I removed Default is "highest" as it was redundant.

thatsmydoing · 2022-09-26T05:32:38Z

src/index.ts

@@ -107,9 +137,15 @@ const computePackageInstances = (packages: Packages, name: string, useMostCommon
    // Extract the list of unique versions for this package
    const versions:Versions = new Map();
    for (const packageInstance of packageInstances) {
-        if (versions.has(packageInstance.installedVersion)) continue;
+        if (versions.has(packageInstance.installedVersion)) {
+            const existingPackage = versions.get(packageInstance.installedVersion)!;


I've moved this down here so only Version needs the isDirectDependency flag. There's no entryName to check in the version sorting step so it's not that easy to do it there. If we absolutely want to avoid having this, I suppose it's possible to loop over satisfies in the sorting function and check if any requested version is a direct dependency but that'd just be worse for performance.

scinos · 2022-12-11T14:06:57Z

Sorry, I haven't been available to review this change. I'll have a look as soon as possible.

burtek · 2023-05-15T09:06:09Z

This would actually be nice to have :)

scinos reviewed Sep 24, 2022

View reviewed changes

thatsmydoing commented Sep 26, 2022

View reviewed changes

Prioritize direct dependency if available

489381a

thatsmydoing force-pushed the prioritize-direct-dependency branch from 0412618 to 489381a Compare September 26, 2022 05:13

thatsmydoing commented Sep 26, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prioritize direct dependency if available #192

Prioritize direct dependency if available #192

thatsmydoing commented Sep 9, 2022

scinos Sep 24, 2022

scinos Sep 24, 2022

thatsmydoing Sep 26, 2022

scinos Sep 24, 2022

scinos commented Sep 24, 2022

scinos Sep 24, 2022

thatsmydoing Sep 26, 2022

thatsmydoing left a comment

thatsmydoing Sep 26, 2022

thatsmydoing left a comment

thatsmydoing Sep 26, 2022

thatsmydoing Sep 26, 2022

scinos commented Dec 11, 2022

burtek commented May 15, 2023

Prioritize direct dependency if available #192

Are you sure you want to change the base?

Prioritize direct dependency if available #192

Conversation

thatsmydoing commented Sep 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scinos commented Sep 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thatsmydoing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thatsmydoing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scinos commented Dec 11, 2022

burtek commented May 15, 2023