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

Add yarn berry compatibility #363

Merged
merged 3 commits into from Sep 28, 2022
Merged

Add yarn berry compatibility #363

merged 3 commits into from Sep 28, 2022

Conversation

maman
Copy link
Contributor

@maman maman commented Dec 3, 2021

Summary

Yarn "berry" introduced a new lockfile format that now adheres to YAML.

The current patch-package's yarn lockfile parser uses @yarn/lockfile - which will throw errors since it's only valid for yarn v1 (or "classic").

This PR adds support to parse the new, yaml-based lockfile. The feature detection is performed by checking the existence of the string # yarn lockfile v1 on the lockfile. If the string is found, then we'll parse it using @yarn/lockfile. if it isn't, then we'll use the yaml package to parse it.

Another thing that we need to adjust is, on the yarn berry lockfile packages from npm are prefixed with "npm:". This PR also adjusts it to remove the prefix, leaving the version number only as expected.

@d0whc3r
Copy link

d0whc3r commented Feb 27, 2022

What about this?

@d0whc3r
Copy link

d0whc3r commented Feb 27, 2022

btw, --create-issue isn't working with these changes and yarn 3

@d0whc3r
Copy link

d0whc3r commented Feb 28, 2022

It worked for me if I add an extra change, in makePatch.ts, before running the install command, https://github.com/ds300/patch-package/blob/master/src/makePatch.ts#L130, I added the "yarn set version classic" command like:

        spawnSafeSync(`yarn`, ["set", "version", "classic"], {
          cwd: tmpRepoNpmRoot,
          logStdErrOnError: false,
        })

With this change, it works

@d0whc3r
Copy link

d0whc3r commented Feb 28, 2022

These are the changes (in build files):

diff --git a/node_modules/patch-package/dist/getPackageResolution.js b/node_modules/patch-package/dist/getPackageResolution.js
index bc7ffaa..969414a 100644
--- a/node_modules/patch-package/dist/getPackageResolution.js
+++ b/node_modules/patch-package/dist/getPackageResolution.js
@@ -9,6 +9,7 @@ const PackageDetails_1 = require("./PackageDetails");
 const detectPackageManager_1 = require("./detectPackageManager");
 const fs_extra_1 = require("fs-extra");
 const lockfile_1 = require("@yarnpkg/lockfile");
+const yaml_1 = require("yaml");
 const find_yarn_workspace_root_1 = __importDefault(require("find-yarn-workspace-root"));
 const getPackageVersion_1 = require("./getPackageVersion");
 function getPackageResolution({ packageDetails, packageManager, appPath, }) {
@@ -24,12 +25,25 @@ function getPackageResolution({ packageDetails, packageManager, appPath, }) {
         if (!fs_extra_1.existsSync(lockFilePath)) {
             throw new Error("Can't find yarn.lock file");
         }
-        const appLockFile = lockfile_1.parse(fs_extra_1.readFileSync(lockFilePath).toString());
-        if (appLockFile.type !== "success") {
-            throw new Error("Can't parse lock file");
+        const lockFileString = fs_extra_1.readFileSync(lockFilePath).toString();
+        let appLockFile;
+        if (lockFileString.includes("yarn lockfile v1")) {
+          const parsedYarnLockFile = lockfile_1.parse(lockFileString)
+          if (parsedYarnLockFile.type !== "success") {
+            throw new Error("Can't parse lock file")
+          } else {
+            appLockFile = parsedYarnLockFile.object
+          }
+        } else {
+          try {
+            appLockFile = yaml_1.parse(lockFileString)
+          } catch (e) {
+            console.error(e)
+            throw new Error("Can't parse lock file")
+          }
         }
         const installedVersion = getPackageVersion_1.getPackageVersion(path_1.join(path_1.resolve(appPath, packageDetails.path), "package.json"));
-        const entries = Object.entries(appLockFile.object).filter(([k, v]) => k.startsWith(packageDetails.name + "@") &&
+        const entries = Object.entries(appLockFile).filter(([k, v]) => k.startsWith(packageDetails.name + "@") &&
             v.version === installedVersion);
         const resolutions = entries.map(([_, v]) => {
             return v.resolved;
@@ -49,6 +63,9 @@ function getPackageResolution({ packageDetails, packageManager, appPath, }) {
         if (resolution.startsWith("file:.")) {
             return `file:${path_1.resolve(appPath, resolution.slice("file:".length))}`;
         }
+        if (resolution.startsWith("npm:")) {
+          return resolution.replace("npm:", "")
+        }
         return resolution;
     }
     else {
diff --git a/node_modules/patch-package/dist/makePatch.js b/node_modules/patch-package/dist/makePatch.js
index 985589e..e8704dd 100644
--- a/node_modules/patch-package/dist/makePatch.js
+++ b/node_modules/patch-package/dist/makePatch.js
@@ -67,6 +67,10 @@ function makePatch({ packagePathSpecifier, appPath, packageManager, includePaths
         if (packageManager === "yarn") {
             console.info(chalk_1.default.grey("•"), `Installing ${packageDetails.name}@${packageVersion} with yarn`);
             try {
+              spawnSafe_1.spawnSafeSync(`yarn`, ["set", "version", "classic"], {
+                  cwd: tmpRepoNpmRoot,
+                  logStdErrOnError: false,
+              });
                 // try first without ignoring scripts in case they are required
                 // this works in 99.99% of cases
                 spawnSafe_1.spawnSafeSync(`yarn`, ["install", "--ignore-engines"], {

Also I added yaml as a dependency of my own project, just to have it in node_modules

@milesj
Copy link

milesj commented Mar 18, 2022

Any movement on this?

@FilDevTronic
Copy link

@ds300 are you still maintaining this project?

1 similar comment
@ShaneZhengNZ
Copy link

@ds300 are you still maintaining this project?

@milesj
Copy link

milesj commented Aug 8, 2022

Yarn 2/3 has built-in patching, you don't need this package: https://yarnpkg.com/cli/patch

@FilDevTronic
Copy link

FilDevTronic commented Aug 8, 2022

Yarn 2/3 has built-in patching, you don't need this package: https://yarnpkg.com/cli/patch

Sure - if you don't want the features of patch-package then go use the in-built yarn patch. This isn't news.

@milesj
Copy link

milesj commented Aug 8, 2022

@FilipMaslovaric It may be news to people who are unaware that Yarn supports this natively now.

Furthermore, yarn patches and the patches produced by patch-package are not the same, so you run the risk of mutating yarn's node modules in an incorrect way. Just letting people know, no need to be snarky.

@FilDevTronic
Copy link

FilDevTronic commented Aug 9, 2022

@FilipMaslovaric It may be news to people who are unaware that Yarn supports this natively now.

Furthermore, yarn patches and the patches produced by patch-package are not the same, so you run the risk of mutating yarn's node modules in an incorrect way. Just letting people know, no need to be snarky.

The issues of the two not producing compatible changes is a non sequitur to what I said.

Obviously it's news if you didn't know about it, but in and of itself, isn't news. Sorry you just found out.

It's misleading to say that you "don't need" patch-package simply because Yarn has its own yarn patch, because you're assuming that everyone is happy to forego the features that patch-package has, that yarn patch does not.

You're may mislead others with what you're "just letting [them] know".

Further clarification (29/09/2022):

The reason why you absolutely should be using patch-package over yarn patch if you can, is that yarn patch doesn't concern itself with telling you if a change in the package being patched may have invalidated the patch itself, or inversely if your patch is now going to break things. patch-package in @ds300 wisdom, does this very useful thing, and will scream at you in your console if the patch is now incompatible since the package was updated, or even if there was just an update, but the patch doesn't seem to be incompatible (nudging you to double-check, and just update the version of the package that the patch file is pointing to).

If you happen to be in a situation where your project is moving faster than the community can patch or properly fix open source libraries, or even if some other internal dependent project isn't quite ready to publish changes, and you are patching things relatively frequently as an interim-solution, then this package is an absolute lifesaver as it can be easier to forget that something was indeed patched when the evidence of said patch is buried in a somewhat large package.json file whose section you may not visit very often in your daily work.

Would love for yarn patch to adopt some of these features, as going with first-party solutions that are more integrated generally "should" be a better choice, but in this instance, this little gem is far superior.

@kylerjensen
Copy link

@ds300 could you merge this PR please?

src/getPackageResolution.ts Outdated Show resolved Hide resolved
src/getPackageResolution.ts Outdated Show resolved Hide resolved
@orta
Copy link
Collaborator

orta commented Sep 28, 2022

Cool, thanks 👍🏻

@orta orta merged commit bb628da into ds300:master Sep 28, 2022
@FilDevTronic
Copy link

@orta you are a life saver. Thank you. 🙇 @maman thanks for getting us this fix 🙏

@kylerjensen
Copy link

@orta Thanks for merging this PR! Do you know when we can expect a new version to be published to NPM?

@ds300
Copy link
Owner

ds300 commented Oct 25, 2022

This was just released in v6.5.0! Thanks for your contribution 🙏🏼 🎉

@maman maman deleted the feature/add-yarn-berry-compatibility branch December 19, 2022 04:47
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.

None yet

8 participants