ds300 / patch-package

Fix broken node modules instantly 🏃🏽‍♀️💨
MIT License
10.26k stars 287 forks source link

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

Closed anas10 closed 1 year ago

anas10 commented 1 year ago

Resolves https://github.com/ds300/patch-package/issues/393

julestruong commented 1 year ago

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

orta commented 1 year ago

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 commented 1 year ago

I used patch package and yes it works

anas10 commented 1 year ago

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

anas10 commented 1 year ago

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 ✅

orta commented 1 year ago

No idea why - but I think we might need this on CI, https://github.com/grover/homebridge-dacp/issues/66#issuecomment-592986242

iTonyYo commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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

orta commented 1 year ago

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 commented 1 year ago

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

orta commented 1 year ago

Thanks everyone! Looks good to me

ds300 commented 1 year ago

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