-
-
Notifications
You must be signed in to change notification settings - Fork 302
feat(packageresolution): add lockfileVersion 2+ packages support #434
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
feat(packageresolution): add lockfileVersion 2+ packages support #434
Conversation
good job :) when do you plan to merge this ? |
Cool - did you verify it works @julestruong? @anas10 it'd be great to get an integration test for this, because I don't know enough about npm's lock files to know if it is correct: https://github.com/ds300/patch-package/tree/master/integration-tests (and to make sure no-one breaks it further down the line) |
I used patch package and yes it works |
@orta Good shout, I've now added some integration tests for every version of lockfile. |
Tests updated. I've made sure that they were not false positives. |
npm i $1 | ||
alias patch-package=./node_modules/.bin/patch-package | ||
|
||
function testLockFile() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps this syntax is only available on your machine and not on CI? Assuming you're using a mac, it defaults to zsh - and I bet CI defalts to bash
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's odd. This is a fairly standard syntax 🤔
I've made a small change, can you trigger now to see. If not, I'll just repeat the test 3 times without the function.
4786489
to
66184c6
Compare
No idea why - but I think we might need this on CI, grover/homebridge-dacp#66 (comment) |
Why not just judge by const lockfile = require(path_1.join(appPath, packageManager === "npm-shrinkwrap"
? "npm-shrinkwrap.json"
: "package-lock.json"));
if (lockfile.lockfileVersion > 2) {
return Object.entries(lockfile.packages).find(el => el[0].includes(packageDetails.name))[1].resolved;
}
const lockFileStack = [lockfile];
for (const name of packageDetails.packageNames.slice(0, -1)) {
const child = lockFileStack[0].dependencies;
if (child && name in child) {
lockFileStack.push(child[name]);
}
}
lockFileStack.reverse();
const relevantStackEntry = lockFileStack.find((entry) => entry.dependencies && packageDetails.name in entry.dependencies);
const pkg = relevantStackEntry.dependencies[packageDetails.name];
return pkg.resolved || pkg.from || pkg.version; |
@anas10 Just delete This is because bash does not expand aliases when in non-interactive mode and shopt succeed: #!/bin/sh
alias my-command='echo hello'
my-command
# => hello fail: #!/bin/bash
alias my-command='echo hello'
my-command
# => line 3: my-command: command not found |
Other problems: While not critical, it might be a good idea to install Now lockfile v2 and v3 work. However, v1 still does not work.
To fix this, adding I don't know how to resolve this. As a workaround, we can avoid the CI error by deleting the lockfile v1 test. How I fixed it: https://github.com/Threestup/patch-package/pull/1/files |
@orta Tests are updated, can you retrigger the tests? |
This PR looks good! I'm not at my main computer for 2 weeks, so I can't really handle the merge conflict for you - it should probably be automatic, it's changing a line next to one of your changes. If you get that fixed, I'll merge the PR and get a release sorted! |
6e0641f
to
8999185
Compare
@orta I've resolved the conflicts. Thanks for checking |
Thanks everyone! Looks good to me |
Released in v6.5.1, thanks for the contribution 🎉 ❤️ |
Resolves #393