Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: overrides #129

Closed
wants to merge 2 commits into from
Closed

RFC: overrides #129

wants to merge 2 commits into from

Conversation

isaacs
Copy link
Contributor

@isaacs isaacs commented Apr 10, 2020

@isaacs isaacs force-pushed the isaacs/overrides branch 2 times, most recently from 6e737d4 to bb4fc14 Compare April 10, 2020 07:30

## Questions and Bikeshedding

- Should `bundleDependencies` and `shrinkwrap` dependencies be subject to
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would not expect bundleDeps to be affected, since the whole point of them is "npm is not allowed to touch bundled deps". Not sure what you mean about "shrinkwrap deps"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dependencies of a module that includes a npm-shrinkwrap.json file. Again, the whole point of them is "npm is not allowed to use different deps than the tree specified in the shrinkwrap", so in most ways, they're treated just like bundled deps (albeit, bundled deps that we still have to fetch and unpack).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to me like that shouldn't be touched either.

@isaacs isaacs force-pushed the isaacs/overrides branch 2 times, most recently from 8dda8ca to d556357 Compare April 10, 2020 07:54
@ruyadorno
Copy link
Contributor

I wanted to see an example for overriding a dep version and also overriding versions for some of its sub deps (I can see it being a requirement to many plugin-based ecosystems) e.g:

{
  "overrides": {
    "y": "2.x",
    "y@2.x": {
      "x@1.x": "1.2.3"
    }
  }
}

@isaacs
Copy link
Contributor Author

isaacs commented Apr 14, 2020

I wanted to see an example for overriding a dep version and also overriding versions for some of its sub deps (I can see it being a requirement to many plugin-based ecosystems) e.g:

{
  "overrides": {
    "y": "2.x",
    "y@2.x": {
      "x@1.x": "1.2.3"
    }
  }
}

Oh, that's interesting, and might run into issues with the "no re-evaluating" rule suggested in the RFC.

Let's say you have a y@1 dep, which gets rewritten to y@2.x. Then, because that was overridden already, it won't get the overrides attached to it. So, you could have y@1 get overridden to y@2, but then get x@1 getting version 1.9.9 instead of 1.2.3.

Maybe it should get object overrides attached to it, but not string overrides, so we don't have infinite loops?

Eg, this would not work:

{
  "overrides": {
    "y@1": "2",
    "y@2": "2.3.4",
    "y@2.3.4": "1.2.3" // uh oh
  }
}

A y@1 dep gets rewritten to y@2 which resolves to y@2.9.9, which matches y@2 and gets rewritten to y@2.3.4, which matches y@2.3.4 and gets rewritten to y@1.2.3 which matches y@1 and now we're in trouble.

But in this case:

{
  "overrides": {
    "y@1": "2",
    "y@2": {
      "x@1": "1.2.3"
    }
}

The y@1.x dep gets rewritten to y@2 which resolves to y@2.3.4. This matches y@2 which is an object value, so it gets attached to the node. Then its dependents matching x@1 get rewritten to x@1.2.3.

If we block string overriding to a single move, then this wouldn't work:

{
  "overrides": {
    "y@1": "2",
    "y@2": "2.3.4",
    "y@2.3.4": {
      "x@1": "1.2.3"
    }
}

The y@1 dep gets rewritten to y@2, which resolves to y@2.9.9. This matches y@2, but the value there is a string, and it's already been overridden so we give up. y@2.9.9 then has no overrides attached, and gets x@1.9.9, which is not what the user expects. They'd have to do this instead:

{
  "overrides": {
    "y@1": "2.3.4",
    "y@2": "2.3.4",
    "y@2.3.4": {
      "x@1": "1.2.3"
    }
}

We could perhaps handle the infinite loop string-value scenario by tracking which override rules were applied, and only applying each rule a single time. So in the loop case:

{
  "overrides": {
    "y@1": "2",
    "y@2": "2.3.4",
    "y@2.3.4": "1.2.3" // uh oh
  }
}

The y@1 dep gets rewritten to 2, which resolves to 2.9.9. This matches the y@2 rule, and gets rewritten to 2.3.4. This matches the y@2.3.4 rule, and gets rewritten to 1.2.3. The y@1 rule is not considered, because it was already applied to this dep, so it ends up as y@1.2.3.

If you started with y@2.3.4, it'd get rewritten to 1.2.3, match y@1 rewritten to y@2 resolved to 2.9.9, match y@2 rewritten to y@2.3.4, and we're done. (y@2.3.4 -> y@2.3.4, not exactly "overridden".)

We might be able to do something clever to resolve these rules, detecting that the range 2 is a subset of 2, then 2.3.4 is a subset of 2.3.4, then 1.2.3 is a subset of 1, so we have a loop. But if the user is putting a tarball or git url on the right hand side, that doesn't work.

@wesleytodd
Copy link

wesleytodd commented Apr 22, 2020

Overall I think this is looking good to me. The only thing which I dont like much (although I don't consider this a blocker), is the case here: https://github.com/npm/rfcs/pull/129/files#diff-8a9b6b0e44ed001c48613a8c6d924cf9R309-R324

{
  "y": "1.2.3",
  "y@1": {
    "x": "1.2.3"
  }
}

I am not sure I like the "just use any other specifier key to avoid the fact that you mean the same thing". What if instead it was something like this?

{
  "y": {
    ".": "1.2.3",
    "x": "1.2.3"
  }
}

Some signifier which references the 'parent specifier' would better express the intent of the user IMO. Going to the edge cases it could even allow for something like this:

{
  "y": {
    ".": "1.2.3",
    "x": "1.2.3",

    ".@2": {
      ".": "2.0.1",
      "x": "2.0.0"
    }
  }
}

I know this is pushing back to the complex area, and maybe the nesting objects is out of scope, but for the simple case I feel like saying "I want 1.2.3 of y, and also 1.2.3 of x under y" is more clear with this than by requiring a semver range just to ensure it is not a duplicate key.

@isaacs
Copy link
Contributor Author

isaacs commented Apr 23, 2020

Yeah, I kind of like . as an option to avoid the double-match. (It also lets us go back to saying "first match wins, no combining", which simplifies the logic quite a bit.)

I very much do not like .@<specifier> to further limit it. I think if you want separate objects for y@1 and y@2, you just create two objects. So, that'd change your third example to:

{
  "y@1": {
    ".": "1.2.3",
    "x": "1.2.3"
  },
  "y@2": {
    ".": "2.0.1",
    "x": "2.0.0"
  }
}

I'd go another step further on the "." point and say that "." may only be a string value, not an object.

I think that's a little bit less confusing to read, and avoids the following nightmare:

{
  "y": {
    ".@1": {
      ".@1.2": {
        ".@1.2.5": {
          ".": "1.2.4"
          "x": {
            ".@2": "2.0.1",
            ".": "2.0.2"
          }
        },
        ".@1.2.3": "1.2.4"
      },
      "thenight": {
        ".": {
          "is": "dark",
          ".": "1.2.3"
        },
        ".@and": {
           ".": "full",
           "of": {
             ".": "terrors"
           }
         }
      },
      ".": "1.2.7"
    }
  }
}
// quick, without cheating, what gets rewritten to 1.2.7?

@isaacs
Copy link
Contributor Author

isaacs commented Apr 23, 2020

Or, I guess the intent of that third example would be more like:

{
  "y@2": {
    ".": "2.0.1",
    "x": "2.0.0"
  },
  "y": {
    ".": "1.2.3",
    "x": "1.2.3"
  }
}

So that all y other than y@2 get the second block applied.

@isaacs
Copy link
Contributor Author

isaacs commented Apr 23, 2020

Then we could also say that "<selector spec>": "<override spec>" is actually a shorthand for "<selector spec>": { ".": "<override spec>" } which has some nice implications for the implementation. We just always use the first object that matches (merged with all parents), and instead of string values having different treatment, the "." key has different treatment.

@wesleytodd
Copy link

I very much do not like .@ to further limit it. I think if you want separate objects for y@1 and y@2, you just create two objects.

I'd go another step further on the "." point and say that "." may only be a string value, not an object.

Sounds good to me!

@ruyadorno
Copy link
Contributor

ruyadorno commented May 20, 2020

following up from the discussion at: #78 (reply in thread)

@isaacs do you think in its current state the proposal from this RFC enables that specific usecase?

@zkochan
Copy link

zkochan commented May 29, 2020

The "." syntax seems very unusual to me. What about using an array instead?

{
  "y@1": [
    "1.2.3",
    {
      "x": "1.2.3"
    }
  ],
  "y@2": [
    "2.0.1",
    {
      "x": "2.0.0"
    }
  ]
}


When the value of an override rule is a string, it is a replacement
resolution target for resolutions matching the key. String values _must_
be a dependency specifier without a name. (Aliases are supported using the
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not clear to me how are aliases treated.

For this case for instance:

{
  "overrides": {
    "y@1": "1.2.3",
    "y@1.2.3": "2.3.4" // <-- this will never match anything
  }
}

What will this override?

Will it override this: "y": "npm:x@1.0.0"
or this "x": "npm:y@1.0.0"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neither of these would be overridden by this rule (at least, as the RFC is written here), because neither would match y@1.

It is interesting, though, and perhaps worth spelling out explicitly in the RFC, how aliases would work.

At least as I read it, if you had an override like this:

{
  "y@npm:x@1": "npm:x@1.2.3"
}

Then an installation of y@npm:x@1.9.9 would be overridden to y@npm:x@1.2.3, because y@npm:x@1.9.9 is a match to y@npm:x@1.


If the first match for a given resolution is an object, then the object is
a new rule set applied to all resolutions down that path in the dependency
graph.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all resolutions down that path in the dependency graph

so in the example below, bar may be not a direct dependency of foo?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct. It would be any instance of bar at any point in the dependency graph traversal from any foo node.

"boo": "3.0.0"
},
"boo": "1.0.0"
}
Copy link

@zkochan zkochan May 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yarn also allows these complex overrides but I can't imagine a real-world example when you'd want this.

Also, I don't think we could support something like this in pnpm.

We could support only overrides when the direct dependent is specified.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see, because you might have two instances of the same version of a package subject to different overrides? Eg:

root -> (a, b)
a -> (x@1.2.3)
b -> (x@1.2.3)
x -> (y)

overrides: {
  a: {
    x: {
      y: "1.2.3"
    }
  },
  b: {
    x: {
      y: "2.3.4"
    }
  }
}

So, the x@1.2.3 depended upon by a would need a y of 1.2.3, but the x@1.2.3 depended upon by b would need y@2.3.4.

Meaning both root -> a -> x and root -> b -> x could not be symbolic links to the same physical x@1.2.3 folder. The realpath target would have to be unique to both the package resolution and its set of overrides.

Am I understanding that correctly? If so, it seems like overrides is a challenge for pnpm even without applying to the full dependency subgraph. If not (and making the real paths unique to both resolution and override set is doable) then I'm not sure how this override is any different:

{
  a: { y: "1.2.3" },
  b: { y: "2.3.4" }
}

Copy link

@zkochan zkochan Jun 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If so, it seems like overrides is a challenge for pnpm even without applying to the full dependency subgraph.

Overrides are doable with pnpm if a particular version of a package gets the same set of dependencies. I don't see issues with your last example. Pnpm would create a directory structure like this:

.pnpm
+ a@1.0.0
   + node_modules/y -> ../../y@1.2.3
+ b@1.0.0
   + node_modules/y -> ../../y@2.3.4
+ y@2.3.4
+ y@1.2.3

But in your first example, we would need to create separate directories for the same x. Something like

.pnpm
+ x@1.2.3_<hash1>
   + node_modules/y -> ../../y@1.2.3
+ x@1.2.3_<hash2>
   + node_modules/y -> ../../y@2.3.4
+ y@2.3.4
+ y@1.2.3

Note: we actually do this for peer dependencies... but I am not sure it is worth adding this for overrides. At the moment, I don't believe overrides need to be so powerful as in this RFC (or as in Yarn).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like overrides would be doable in pnpm if a unique combination of (1) resolved version and (2) override set gets the same set of dependencies (which it would).

If you don't feel it's worthwhile to implement the feature in that way, that's really a trade-off between UX, power, and complexity which is really your choice to make. However, the fact that it can be implemented by pnpm makes me feel like this should not be an obstacle to accepting this RFC and npm implementing it with these semantics.

@isaacs
Copy link
Contributor Author

isaacs commented May 31, 2020

@ruyadorno

following up from the discussion at: #78 (reply in thread)

@isaacs do you think in its current state the proposal from this RFC enables that specific usecase?

No, it doesn't suggest any way to say "do not resolve this dependency". I'm not opposed to that in principle, but it does complicate things somewhat. (Eg, if the dependency is not optional, then it won't be installable, and it might not be obvious which version of a given thing would be installable, if for example the blocked dep was added as a meta-dep in a minor update.)

@zkochan

The "." syntax seems very unusual to me. What about using an array instead?

It's a bikeshed, and I can see that it's functionally equivalent. However, it is somewhat nice to be able to just check if it's an object or string, and not have to also check for it being an Array. Since the Array could only reasonably contain 2 members, it feels like a violation of the zero-one-infinity rule. (Eg, what happens if you do {"foo": [{"bar":"1.2.3"},{"bar":"2.3.4"},"3.0.0","4.0.0"]}? How would we interpret that intent?)

Is there a key name that you can suggest to indicate "the current point in the traversal" that would be less unusual to you than ".", and also not be a valid package specifier?

I had thought maybe "@" would be somewhat intuitive, since it's kind of saying <name>@<version>.

{
  "overrides": {
    "foo": {
      "@": "2.3.4",
      "bar": "1.2.3"
    }
  }
}

@dominykas
Copy link

Just curious if anyone has seen a userland implementation of this (aside from Yarn's equivalent feature)? I figure it would be quite handy to try it out and that could maybe feed back into the spec?

@gajus
Copy link

gajus commented Nov 26, 2020

Just curious if anyone has seen a userland implementation of this (aside from Yarn's equivalent feature)? I figure it would be quite handy to try it out and that could maybe feed back into the spec?

Not the spec implementation, but if anyone needs a workaround, I've documented a solution compatible with NPM v7.

https://stackoverflow.com/a/65014694/368691

@Artur-
Copy link

Artur- commented Jan 14, 2021

Is there any way to follow the progress of the implementation of this? #129 (comment) only mentions that it "will be added in the 7.x line"

@bnb
Copy link

bnb commented Apr 14, 2021

@Artur- there was discussion about this in today's RFC. There was an assertion that the CLI team's focus has been on workspaces, but there are plans to work on it soon (my assumption is that "soon" is "as workspaces work is finished up", which does seem to be happening pretty aggressively).

@isaacs isaacs closed this in 58f92e8 Apr 20, 2021
@isaacs
Copy link
Contributor Author

isaacs commented Apr 20, 2021

Moved to accepted.

@csvan
Copy link

csvan commented Jul 23, 2021

Is there a way to track the status of this being implemented in NPM? It's currently a blocker for us migrating our codebase completely from Yarn and we would love to see it :)

@darrinmn9
Copy link

Is there a way to track the status of this being implemented in NPM? It's currently a blocker for us migrating our codebase completely from Yarn and we would love to see it :)

Would love an answer to this ^. I too will be migrating my company from yarn to npm as soon as this feature is released.

@isaacs
Copy link
Contributor Author

isaacs commented Aug 26, 2021

We've just started sketching out the implementation and will have updates in https://github.com/npm/arborist soon with a tracking issue and some future steps, once we have a slightly clearer idea of what those will be.

Also, please check this out: #441

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.