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

feat(packageresolution): add lockfileVersion 2+ packages support #434

Merged
merged 14 commits into from Dec 29, 2022

Conversation

anas10
Copy link

@anas10 anas10 commented Nov 15, 2022

Resolves #393

@julestruong
Copy link

good job :) when do you plan to merge this ?

@orta
Copy link
Collaborator

orta commented Nov 25, 2022

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)

@julestruong
Copy link

I used patch package and yes it works

@anas10
Copy link
Author

anas10 commented Dec 2, 2022

@orta Good shout, I've now added some integration tests for every version of lockfile.

@anas10
Copy link
Author

anas10 commented Dec 2, 2022

Tests updated. I've made sure that they were not false positives.
Tests fail as expected without the code change and succeed consistently with the code change ✅

npm i $1
alias patch-package=./node_modules/.bin/patch-package

function testLockFile() {
Copy link
Collaborator

@orta orta Dec 2, 2022

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

Copy link
Author

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.

@anas10 anas10 force-pushed the feat/support-newer-lockfile-versions branch from 4786489 to 66184c6 Compare December 2, 2022 15:18
@orta
Copy link
Collaborator

orta commented Dec 2, 2022

No idea why - but I think we might need this on CI, grover/homebridge-dacp#66 (comment)

@iTonyYo
Copy link

iTonyYo commented Dec 8, 2022

Why not just judge by lockfileVersion

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;

@rokiyama
Copy link
Contributor

@anas10 Just delete #!/usr/bin/env bash and CI will succeed. Alternatively, changing it to #!/bin/sh will also work.

This is because bash does not expand aliases when in non-interactive mode and shopt expand_aliases is not set.

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

@rokiyama
Copy link
Contributor

Other problems:

While not critical, it might be a good idea to install left-pad@1.3.0.

Now lockfile v2 and v3 work. However, v1 still does not work.

npm i --lockfile-version 1 produces error: ... /src/mdns.hpp:31:10: fatal error: dns_sd.h: No such file or directory.

To fix this, adding sudo apt install libavahi-compat-libdnssd-dev to main.yaml results in another error: ... /src/mdns_utils.hpp:14:5: error: 'Handle' in namespace 'v8' does not name a template type

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

@rokiyama
Copy link
Contributor

@orta Tests are updated, can you retrigger the tests?

@orta
Copy link
Collaborator

orta commented Dec 28, 2022

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!

@anas10 anas10 force-pushed the feat/support-newer-lockfile-versions branch from 6e0641f to 8999185 Compare December 28, 2022 13:44
@anas10
Copy link
Author

anas10 commented Dec 28, 2022

@orta I've resolved the conflicts. Thanks for checking

@orta
Copy link
Collaborator

orta commented Dec 29, 2022

Thanks everyone! Looks good to me

@orta orta merged commit 59cf2e8 into ds300:master Dec 29, 2022
@ds300
Copy link
Owner

ds300 commented Jan 3, 2023

Released in v6.5.1, thanks for the contribution 🎉 ❤️

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.

[npm] Support lockfile version 3
6 participants