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

make work with pnpm and git, use pnpm-lock.yaml #302

Closed
wants to merge 16 commits into from

Conversation

milahu
Copy link
Contributor

@milahu milahu commented Mar 18, 2021

part of issue #35 (comment)

error was

**ERROR** No package-lock.json, npm-shrinkwrap.json, or yarn.lock file.

install

to install my patched version of patch-package

pnpm rm patch-package
pnpm install @milahu/patch-package
npx patch-package

why is this PR still open?

the author of patch-package has abandoned the project,
so nobody has write access to the repo. feel free to fork my fork

changes

  • use pnpm-lock.yaml to get the resolvedVersion
  • use pnpm to install the origin package (if pnpm-lock.yaml was found)
  • workaround for pnpm symlinks
  • add some debug output, also show the output of npm install etc (todo: add a --verbose flag to CLI)
  • exclude lockfiles from diff. maybe exclude more files with patch-package --exclude '^dist/.*'
  • fix some lint errors via tslint --fix and prettier --write (sorry for the diff noise)
  • allow comment header in patch files, see my comment
  • support pnpm's link:../../../some/relative/path
    • detect local git repos
      • patch is based on the origin/HEAD commit
      • use full commit hash as version for npm
      • as fallback, use version from local package.json
    • dont copy node_packages folders from local package

todo

are git patches applied correctly? we must install the exact commit

fixme

when we install packages from git (pinned to commit), patch-package will say
Warning: patch-package detected a patch file version mismatch
cos the patch version is the "declared" version from /package.json = commit hash
= for example 8a5162785c58864ef65308c0f0d890cade0b407f
and the "applied to" version ("resolved" version) is from node_modules/pkg/package.json
= for example 1.0.0-canary.10
both versions (declared and resolved) could be stored in the comment header of the patch file

@milahu

This comment has been minimized.

@worstpractice
Copy link

@milahu Awesome work! I was just about to look into this myself when I found your PR.

@PabloSzx
Copy link

@ds300 Is it possible to take a look into this PR? thank you

@milahu
Copy link
Contributor Author

milahu commented May 11, 2021

thx for the keepalive ; )

thing is, the main patch-package repo is slightly undermaintained ...
i mean, my last patch (#252) was 3 lines, and that took 190 days to merge.
looks like ds300 is the only maintainer and shifts the backlog only like 2 or 3 times per year : /

i never really came to use my patch, let me catch up on that:

install patch-package branch pnpm-support-2
cd /my/fancy/node/project
mkdir patched_packages
cd patched_packages
git clone --depth 1 https://github.com/milahu/patch-package.git --branch pnpm-support-2
cd patch-package
pnpm install
pnpm run build
cd ../../
node patched_modules/patch-package/dist/index.js some-node-package
tests

edit: github CI says tests passed 0__o let me run tests again ...
edit 2: nope, still the same error:

# if you have some minutes to waste ...
pnpm run test
# jest --runInBand                                                 
# ts-jest[config] (WARN) Unable to find the root of the project where ts-jest has been installed.
# ts-jest[versions] (WARN) Version 4.2.4 of typescript installed has not been tested with ts-jest. If you're experiencing issues, consider using a supported version (>=2.7.0 <4.0.0). Please do not report issues in ts-jest if you are using unsupported versions.
# FAIL src/packageIsDevDependency.test.ts
#   ● packageIsDevDependency › returns false if package is a transitive dependency of a dev dependency
# [ may be related to my patch ]
# Test Suites: 1 failed, 35 passed, 36 total
# Tests:       1 failed, 5 skipped, 804 passed, 810 total
# Snapshots:   65 passed, 65 total
# Time:        438.918s

one problem i found:
when /package.json says .dependencies.some-package = link:../../some/where
then /pnpm-lock.yaml says .dependencies.some-package = 1.2.3
so the "resolved" version (from /pnpm-lock.yaml) is wrong
and my "detect git version" logic in patch-package/getPackageResolution.js is not used
-> added a workaround

another problem, also

when /package.json says .dependencies.some-package = link:../../some/where

... then that /package.json will obviously not work on other machines (not portable)
and we would have to replace link:../../some/where
with the git-version (or npm-version), that the patch is based on
or straight-up throw a fatal error in patch-package when /package.json says link:../../some/where?

opinions on this?
what is a "reasonable default" workflow to combine git and pnpm?

when i do pnpm install git+https://github.com/ds300/patch-package.git
then node_modules/patch-package is not a git repo

when i do

(
  cd /tmp; git clone --depth 1 https://github.com/ds300/patch-package.git
  cd patch-package; pnpm install; pnpm run build
)
pnpm install /tmp/patch-package

then the resolvedVersion in /pnpm-lock.yaml is wrong (not the git version)
and /package.json is not portable

so ... imho the only solution is to make pnpm clone the git repo
by running something like pnpm install --git-clone git+https://github.com/ds300/patch-package.git
this would clone with a default depth of 1, allowing to set --git-depth <depth>

ideally, following runs of pnpm install or pnpm remove should not change the git repo

@PabloSzx
Copy link

when /package.json says .dependencies.some-package = link:../../some/where

... then that /package.json will obviously not work on other machines (not portable)
and we would have to replace link:../../some/where
with the git-version (or npm-version), that the patch is based on
or straight-up throw a fatal error in patch-package when /package.json says link:../../some/where?

opinions on this?
what is a "reasonable default" workflow to combine git and pnpm?

I feel like using patch-package on a locally linked package is way too niche, and if it takes way too much work or maintenance in the future just to keep it working, it should just throw, I'm pretty sure the 99% of the usage of patch-package is just for npm packages.

@milahu
Copy link
Contributor Author

milahu commented May 12, 2021

it should just throw

agree, linked packages are not portable

I'm pretty sure the 99% of the usage of patch-package is just for npm packages.

... but some changes are non-trivial
for example when i want to cherry-pick some unmerged PRs (i usually create PRs for my patches)
but i want to publish my "private merge" branch via patch-package, not via git

my workflow can be solved with

1. in the npm project, install some-package from git. pin the version with the full commit hash
we can install the latest version from npm, but then the diff is larger
2. in a separate folder, clone the git repo of some-package, make changes, branches, commits, ...
3. copy the patched files to the npm project's node_packages/some-package/
sample: rsync --archive /tmp/repo/some-package/ /tmp/project/node_packages/some-package/
4. run patch-package some-package

step 3 could be automated with a file watcher, e.g. chokidar.watch

another detail (see next comment)
the version of the patched package should be pinned in /package.json (single source of truth)
so it works with all package managers at all times

@milahu
Copy link
Contributor Author

milahu commented May 12, 2021

done: restore the old getPackageVersion for file protocol (to fix tests),
and for other protocols (http://some/file.tgz#sha1sumoftgzfile, etc.)

when the package is installed from git, patch-package will require to pin the version in package.json:

{ 
  "dependencies": {
    "some-package": "git://some/where#fullcommithash"
  }
}

todo: automate the version-pinning with a --pin-version flag for patch-package
-> will edit package.json to pin the latest version

discuss: is this "version-pinning in package.json" too pedantic?
should we "just create a patch" and complain later on version mismatch?
currently applyPatches.ts will say
Warning: patch-package detected a patch file version mismatch

todo: fix installedPackageVersion in applyPatches.ts -> should use the commit hash for git repos
either from declaredVersion, or from git rev-parse HEAD

fixed: the --debug flag consumed the next argv
replaced the unmaintained minimist argv parser with dashdash

tests: to test npm packages, we could run a local npm registry

done: add header comments to the generated patch files. sample:

# generated by patch-package 6.4.7 on 2021-05-12 20:07:53
#
# command:
#   npx patch-package --exclude '^test/stubs.*' @11ty/eleventy
#
# declared package:
#   @11ty/eleventy: github:11ty/eleventy#8a5162785c58864ef65308c0f0d890cade0b407f
#
# header comments also make room for custom "patch messages", like ...
#
# this patch will
#
#  * add feature xyz
#  * fix bug 123
#
diff --git a/node_modules/@11ty/eleventy/src/Engines/Txt.js b/node_modules/@11ty/eleventy/src/Engines/Txt.js
new file mode 100644
index 0000000..aef38ca
--- /dev/null
+++ b/node_modules/@11ty/eleventy/src/Engines/Txt.js
@@ -0,0 +1,34 @@
+// based on Html.js
+
+const TemplateEngine = require("./TemplateEngine");
...

todo: when package is a git repo, add commit messages as diff header comments

todo: when an old patch exists, prepend the old header comments to the new patch

done: add prepare script to auto-build on install from source

pnpm i github:milahu/patch-package#pnpm-support-2 # requires pnpm 6.2.2
npx patch-package

todo: emulate git status to preview what files will be changed or added -> exclude unwanted files before makePatch

@Jack-Works
Copy link

hi @ds300 can you review this? thanks!

Comment on lines +97 to +98
// TODO dont use lockfiles at all?
// package versions should be pinned in /package.json, so it works with all package managers at all times
Copy link
Contributor Author

@milahu milahu Aug 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

discuss ... pinning versions in /package.json is the simplest and most portable solution.
lockfiles are only needed for nested/recursive/deep/secondary dependencies

@milahu
Copy link
Contributor Author

milahu commented Aug 29, 2021

so ... maintainer is gone, nobody wants to maintain a fork,
so we must install patch-package from some hidden fork branch,
cos we cant use patch-package to patch patch-package : D

pnpm i github:milahu/patch-package#pnpm-support-2 # requires pnpm 6.2.2
npx patch-package

its not perfect, but it works : )

@OmgImAlexis
Copy link

@ds300 can you please review this?

@milahu
Copy link
Contributor Author

milahu commented Jan 2, 2022

thanks for reporting, fixed

# binary release
pnpm install @milahu/patch-package-with-pnpm-support@6.4.8

# source release
pnpm i github:milahu/patch-package-with-pnpm-support
pnpm i github:milahu/patch-package#pnpm-support-2

note: some tests are failing

mmaietta added a commit to electron-userland/electron-builder that referenced this pull request Jan 30, 2022
…electron-updater`. This is to reduce noise on tags being pushed. (Note: Must use custom fork of patch-package until ds300/patch-package#302 is released.)

Activating additional Updater tests
mmaietta added a commit to electron-userland/electron-builder that referenced this pull request Jan 30, 2022
* Patching changesets to only publish tags for `electron-builder` and `electron-updater`. This is to reduce noise on tags being pushed. (Note: Must use custom fork of patch-package until ds300/patch-package#302 is released.)
Activating additional Updater tests
@airtonix
Copy link

airtonix commented Mar 3, 2022

what's the hold up here ?

@milahu
Copy link
Contributor Author

milahu commented Mar 3, 2022

patch-package != pnpm

what's the hold up here ?

patch-package is unmaintained.
feel free to use my pnpm install @milahu/patch-package-with-pnpm-support
and/or maintain your own fork of patch-package

if you allow upstream packages to block your workflow, youre doing something wrong

andresgutgon added a commit to andresgutgon/remix-storage that referenced this pull request Mar 7, 2022
The solution is here:
remix-run/remix#1193 (comment)
And for patching a npm package is this package:
https://www.npmjs.com/package/patch-package

Which doesn'r work with pnpm LOL. Patched itself here:
ds300/patch-package#302 (comment)
andresgutgon added a commit to andresgutgon/remix-storage that referenced this pull request Mar 7, 2022
we can run this code on Remix example apps.
The way it works is you add the package in `/packages` folder. And if
you want a new remix app example add it to `/examples`. It's explained
in the README. Please follow the instructions

Patch @remix-run/dev to allow watching changes on packages
The solution is here:
remix-run/remix#1193 (comment)
And for patching a npm package is this package:
https://www.npmjs.com/package/patch-package

Which doesn't work with pnpm LOL. Patched itself here:
ds300/patch-package#302 (comment)
@weyert
Copy link

weyert commented Mar 17, 2022

patch-package is unmaintained. feel free to use my pnpm install @milahu/patch-package-with-pnpm-support and/or maintain your own fork of patch-package

if you allow upstream packages to block your workflow, youre doing something wrong

Thank you! Any reason why you didn't merge it into main of your fork of the patch-package? :)

@milahu
Copy link
Contributor Author

milahu commented Mar 17, 2022

this PR is merged in https://github.com/milahu/patch-package-with-pnpm-support

@weyert
Copy link

weyert commented Mar 17, 2022

Thank you! Switching over to your version :>

@wighawag
Copy link

Hey @milahu

thanks for your work

this PR is merged in https://github.com/milahu/patch-package-with-pnpm-support

I tried to use it in mono-repo setup but this does not work

if I install at root and execute pnpm patch-package <package-to-patch> it cannot find the package

If I instead install it on the sub folder, it cannot find pnpm-lock.yaml

@milahu
Copy link
Contributor Author

milahu commented Mar 21, 2022

thanks for reporting. pnpm workspaces (monorepos) are not implemented
should be easy to do ... please dont wait for me, i have no time for this
see also #277 and yarn workspaces and npmv7 workspaces

@caschbre
Copy link

caschbre commented Apr 7, 2022

@milahu I found a fork of your repo that adds the pnpm lock file detection for workspaces... in case that is something you could easily roll back into your repo.

https://github.com/Tellus/patch-package-with-pnpm-support

@milahu
Copy link
Contributor Author

milahu commented Apr 8, 2022

thanks @Tellus
thanks @caschbre

published as @milahu/patch-package-with-pnpm-support version 6.4.9

@Tellus
Copy link

Tellus commented Apr 12, 2022

thanks @Tellus

You're perfectly welcome. I mulled issuing a pull request back then but wasn't sure if it was up to code standards.

Glad to have been of help :)

Thanks for the great work on the package in general!

@milahu
Copy link
Contributor Author

milahu commented Apr 14, 2022

bugfix for pnpm workspaces published as @milahu/patch-package-with-pnpm-support@6.4.10
tell me when it breaks ; )

@milahu
Copy link
Contributor Author

milahu commented Apr 22, 2022

renamed to @milahu/patch-package

@milahu/patch-package-with-pnpm-support is deprecated

@weyert
Copy link

weyert commented May 19, 2022

How do people use patch-package with pnpm for a dependency of a workspace package?

@Jack-Works
Copy link

Good news!! PNPM now has official support for yarn style patch!

pnpm/pnpm#2675

We just need to wait for its next release. The benefit of the official support in PNPM is that you won't corrupt the global store (they're shared!)

Also thanks for @milahu, your PR helped us go through the hard time before the official support!

@zkochan
Copy link

zkochan commented Jun 29, 2022

pnpm has a pnpm patch command since v7.4

@orta
Copy link
Collaborator

orta commented Sep 28, 2022

Yeah, I think using the built in pnpm patch command is the right call here. Especially considering the inherent trickiness of editing global package repo which this changes. I think the right call is for us to have a README edit which links to the docs for pnpm patch and we close this PR as being better implemented upstream.

@orta
Copy link
Collaborator

orta commented Sep 28, 2022

Shipped the README change 6fc78ff - and I'm closing this PR.

Folks involved, you did great work and this is not me dismissing that work as not useful - if there are people who can't upgrade to a version of pnpm with support for patching can use @milahu's fork referenced here: #302 (comment)

@orta orta closed this Sep 28, 2022
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.