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

Lock a sub-dependency version #651

Closed
vjpr opened this issue Mar 4, 2017 · 17 comments
Closed

Lock a sub-dependency version #651

vjpr opened this issue Mar 4, 2017 · 17 comments

Comments

@vjpr
Copy link
Contributor

vjpr commented Mar 4, 2017

If a sub-dep releases a version which breaks a dep, this can break an entire app until a bug fix is issued. The workaround is to fork the deps, modify them, publish them, and change the require statments. This a painful, and bites me often.

A way to manually specify a sup-dep version would be very useful, and it is essentially what the shrinkwrap file does.

A command like pnpm lock foo.bar.baz@x.x.x, which would prevent a level 3 dep from updating.

This command would create a key in package.json like:

{
  "lockedDependencies": {
    "foo.bar.baz": "1.0.0",
  }
}

There would then be a warning each time a pnpm install is done to indicate that a dep is being locked.

So its basically like a manual shrinkwrap entry.


If this is too far out of scope for pnpm, it would be cool to have this functionality (manually specifying sub-dep) exposed in the pnpm api so that another tool could implement it.

@zkochan
Copy link
Member

zkochan commented Jul 26, 2017

I like this idea a lot!

Why rewriting by dependency path? Why not just by spec?

{
  "lockedDependencies": {
    "foo@*": "1.0.0",
    "bar@2": "zkochan/bar"
  }
}

@vjpr
Copy link
Contributor Author

vjpr commented Jul 26, 2017

The bug may only apply to a single dep. If you override by version, other deps would update.

The use case for using it would most usually be for a single dep breaking.

Therefore I think both is important.

note: cannot use dot as separator because it is valid in package name.

@andreineculau
Copy link
Contributor

I believe what @vjpr is saying is that he would like to keep whatever works and just handle the errors with precision e.g. dep A has B@* as dependency, B has a major vsn release breaking integration with A, needs locking (although the lock doesn't need to be exact, but just another spec to point to the previous vsn in case it gets patch versions). Dep C has B@1.2.3 as dependency, needs no fixing.

@zkochan
Copy link
Member

zkochan commented Jul 26, 2017

personaly I would always override all but we can support scoping. Something like

{
  "lockedDependencies": {
    "qar@1": {
      "foo@*": "1.0.0"
    },
    "bar@2": "zkochan/bar"
  }
}

looks pretty ugly, I don't know what the format should look like yet 😄

@vjpr
Copy link
Contributor Author

vjpr commented Jul 26, 2017

@zkochan I like that already.

@zkochan
Copy link
Member

zkochan commented Jul 26, 2017

what if I want to rewrite qar@1 as well?

@vjpr
Copy link
Contributor Author

vjpr commented Jul 26, 2017

Hmm, I guess something like this but its a bit ugly - this is why flat might be nicer if we can figure out a way to delimit sub-deps...

"qar@1": {
  "root": "zkochan/qar",
  "bar@2": "zkochan/bar"
}

Some options for flat:

qar@1 > bar@2 <- This is how yarn prints its dep chains. Borrows familiar "direct descendant" css syntax.
qar@1..bar@2
qar@1|bar@2
qar@1$bar@2

@zkochan
Copy link
Member

zkochan commented Jul 26, 2017

I'd suggest just using spaces. > means direct dependency. I think it'd be nicer to not limit ourselves with direct paths, so it'd be

{
  "lockedDependencies": {
    "qar@1 foo@1": "1.0.0",
    "bar@2": "zkochan/bar"
  }
}

@vjpr
Copy link
Contributor Author

vjpr commented Jul 26, 2017

Yeh not needing to specify a direct dep chain would probably make life easier I think.

@zkochan
Copy link
Member

zkochan commented Jul 26, 2017

I wonder if "locked" is the correct word here. Locked would probably be fine if we would only allow specifying a version that satisfies the original range. But we will allow anything, so maybe forcedDependencies or overridenDependencies.

Or even overridenSubDependencies

Or subDependencies?

@zkochan
Copy link
Member

zkochan commented Jul 26, 2017

Had a conversation about this in a npm channel at discord:

Were you thinking about adding the feature of overriding subdepencies to npm? There is an issue at pnpm about this and I really like the idea: #651
In a nutshell, it would allow to force a different version in a subdependency?
If you'd be interested in this, we could try to use the same syntax/configuration(edited)

kat - Today at 7:40 PM
I wouldn't want to do this in package.json
I think this is the sort of thing that should only happen in lockfiles
zkochan - Today at 7:40 PM
but they are autogenerated, no?
kat - Today at 7:41 PM
It should also not allow users to violate semver requirements
Yes, but lockfiles are the ones that can do this kind of manipulation.

Fwiw, npm can already do this. Like, now.
zkochan - Today at 7:43 PM

It should also not allow users to violate semver requirements

so it would not allow to specify a version from a github branch, for instance?(edited)
kat - Today at 7:43 PM
You can easily write a third party tool that just edits your package lock if you're just using a slightly different version. The thing you'd want npm itself for is to reshape the tree if needed
That one's tricky: we have already decided and reasserted that we're never gonna allow replacing dependencies entirely like this, or having aliases.
The git version of the "same" package is still questionable
zkochan - Today at 7:46 PM
seems pretty useful though. If you fixed an issue but you have to wait a lot for it to get merged
like once I had to publish a whole set of packages with different names just because the maintainers were not responding
kat - Today at 7:47 PM
Right
zkochan - Today at 7:48 PM
and also with pnpm we have the issue that sometimes maintainers refuse to make changes "because it works with npm/Yarn"
kat - Today at 7:48 PM
So the only time I'd consider this is if the package has an identical name as well as a compatible version field
And imo the edit should be to the lockfile

@andreineculau
Copy link
Contributor

can I throw in my pennies?

Penny 1. I was also munching through the same thoughts about the lockedDependencies naming and how it's not fully appropriate i.e. maybe a sub-sub-sub dependency with exact spec has a bug, and you want to forcefully upgrade it and use a semver spec instead as well. Can call that a lock.

Penny 2. with all due respect what kat is saying about only allowing this and that, doesn't make sense (see the example I gave just now; that becomes impossible given kat's constraints of following the original semver spec)

Penny 3. but I think he is very much spot on about the information not to be hosted in package.json . Why? Well because overriding something requires knowledge, and that knowledge should only be left to the end-end-end-*-user. The same way lockfiles do not play any part in a dependency, but only on the top-most package (npm install with no args). It has the annoyance/disadvantage that if you publish package foo, you have to instruct whoever is using it to override dependency bar to point to some different spec, but that's fine. After all, I don't want this to become a daily workflow like "yeah, too hard to open up an issue/PR, I'll just have a local package in a folder, with a fix, it will override for whoever installs my package, job done". No! File an issue, send a PR, try to improve the world one comma at a time, don't just create your own ostrich hole!

Penny 4. I'm not saying this should be part of lockfiles either. That forces you to have a lockfile, and while I know some think they are awesome and everyone should have one, the main point I want to raise is that lockfiles derive from package.json, plus sometimes you just want to delete and generate a new one, meaning the information about the overriding doesn't exist YET or gets DELETED.

package.json -> lockfile -> lockfile with overrides does not make sense.

On top of this reason, there's another one: while some lockfiles are nicer, simpler, more readable, git-diff-friendlier, the bottom line is that they are still generated, and a human should not fiddle with it as part of some feature - it is at best a desperate quickfix with top-priority to get a proper fix that will programmatically regenerate the lockfile.

Penny 5. while the package.json doesn't have a proper standard AFAIK, it is still npm that owns its definitions, so introducing generic top-level properties is plain wrong. For all that I know, npm should have a package.json schema check, and barf if it finds out stuff it doesn't know what to make of (imagine if packages out there already had a prepublishOnly script, now comes a newer npm, and they all have to be updated because the semantics where different rolls eyes ). So if pnpm, or any other tool that plays nice with package.json, wants to allow configuration outside of the npm definitions should do it in its own file, as json, as yaml, as md-config, as whatever the author thinks it's cool/appropriate/etc. Whenever that configuration becomes part of the "package.json standard", pnpm may support both, and ideally even deprecate it from its own configuration file specification. The exception to all of this is using package.json's config property i.e. pnpm could grab its own space config.pnpm inside package.json.

PS: I still believe firmly that a callback system opens up more opportunities and closes/skips many of these discussions. I'm only mentioning it, not because I expect somebody else to do it, but because I want people on this thread too to be aware of such a possibility, in case they have more time/knowledge than me to get their hands dirty or provide feedback. After writing the above, I actually just opened up a github issue on this topic so that you see what I mean in greater detail: #861

@zkochan
Copy link
Member

zkochan commented Jul 27, 2017

(Please use "she" when mentioning Kat. The two lead developers of npm are females)

I wasn't saying we're not doing it, I just quoted what npm thinks about it.

I was also leaning towards not having the property in the shrinkwrap file but I don't see so big issue in (Penny 5).

Is your suggestion to ignore this issue and just implement #861?

@andreineculau
Copy link
Contributor

(Please use "she" when mentioning Kat. The two lead developers of npm are females)

my bad, not aware

penny 5

I'm not in the field of discussing size of the issue, just that it is an issue. yarn might also want lockedDependencies with a different format or semantics. What then? fight? I was first? I'm more popular?

#861

yes, or at least prioritize #861 for its simplicity and breadth. this "lockedDependencies" feature could be later added as a convenience because we see that "most times, most people want do just X", though #861 is probably close to "lockedDependencies" in terms of effort and convenience, so it might not be worthwhile

@vjpr
Copy link
Contributor Author

vjpr commented Aug 15, 2017

FYI yarn has implemented it: yarnpkg/yarn#4105

@zkochan
Copy link
Member

zkochan commented Aug 15, 2017

In pnpm this will be most probably supported in some minor version of version 2. Probably in September or October.

@zkochan
Copy link
Member

zkochan commented Sep 2, 2017

I am closing this one as we now have the readPackage hook published with pnpm@1.12.0

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

No branches or pull requests

3 participants