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

update README stating this is technique is not safe for publishing #270

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

davidmurdoch
Copy link

@davidmurdoch davidmurdoch commented Oct 7, 2020

Per #84, it is not safe to publish npm packages with patched dependencies. This notice should be prominent in patch-package documentation.

Note: I don't expect this PR to be merged as it, but rather start a conversation about how to more clearly communicate the use cases for patch-package.

README.md Outdated Show resolved Hide resolved
Co-authored-by: Pedro Augusto de Paula Barbosa <[email protected]>
README.md Outdated
`patch-package` lets app authors instantly make and keep fixes to npm
dependencies. It's a vital band-aid for those of us living on the bleeding edge.

Note for module authors: it is not safe to publish an npm package that uses `patch-package` to patch a non-dev-dependency.
Copy link

@laurent22 laurent22 Oct 23, 2020

Choose a reason for hiding this comment

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

The "why?" part is missing. Either it's obvious why it shouldn't be done, and there's no need to add this to the readme, or it's not, and that should be explained.

My use case is publishing a CLI application and use patch-package to fix some of the dependencies. It has worked fine for years but lately there's a bug and when I search for some info there's the same "it's not safe" being repeated many times, but it's not explained why or what should be the alternative.

Copy link

Choose a reason for hiding this comment

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

Good question. I don't know the why either. I just saw this comment.

Copy link
Author

Choose a reason for hiding this comment

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

I believe it is because patches are applied to a specific file in your node_modules folder, but when a package is installed by end users the node_modules dependency tree may shift things around, and this shifting could be due to factors that can't be controlled by patch-package.

Also, if the dependency you are patching is also imported by the user of your package, patch package shouldn't patch that version of the package... which is not really possible, at least not without some really weird shenanigans.

To work around this issue I used package.json's bundleDependencies to include the actual dependency I patched in my published package.

Copy link

@papb papb Oct 23, 2020

Choose a reason for hiding this comment

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

@davidmurdoch Nice! By the way, I've used bundleDependencies in the past for something else in the past, it works well with npm but I've met an issue with yarn...

Copy link
Author

Choose a reason for hiding this comment

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

Good to know!

Choose a reason for hiding this comment

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

Thanks for clarifying @davidmurdoch, perhaps your comment or a shortened version of it could be included in the readme?

@ds300 ds300 mentioned this pull request Mar 3, 2021
@danlevy1
Copy link

Just wanted to bump this as I didn't catch that this was not supported until I found this thread.

@davidmurdoch davidmurdoch requested review from laurent22 and papb and removed request for papb and laurent22 August 18, 2022 18:01
README.md Outdated Show resolved Hide resolved
Co-authored-by: Pedro Augusto de Paula Barbosa <[email protected]>
@davidmurdoch davidmurdoch requested review from papb and laurent22 and removed request for papb August 18, 2022 21:03
@mbargiel
Copy link

As mentioned by @papb (#270 (comment)), one way to solve the problem when patching the dependencies of a package that you intend to publish is to use bundledDependencies to make sure it's used and also use npm's prepare  script hook rather than postinstall. If you are patching an indirect dependency, you need to bundle the whole dependency chain up to the patched package otherwise the intermediate dependencies may be installed "higher" in the host project's node_modules and end up using an unpatched version of the dependency.

I recommend against making patch-package a production dependency and running it via postinstall in a package you publish, because it introduces the risk that the patching fails due to a different deduped version of the dependency that prevents your patch from applying cleanly. For instance, if your patch applies to [email protected] but [email protected] or higher gets installed in the project, your patch might not apply cleanly and causes installation errors. (Friction for users of your package.) Not to mention that even if it applies cleanly, the patch may be useful to your package, but harmful to other consumers of the shared dependency.

@emmacharp
Copy link

How is this still not included in the README?

It's a very severe limitation! I certainly wish I had known before using this... for it to not work at all in the end. Pardon me for being this straightforward, but I wasted precious time trying to make this work and searching for answers.

Please, for the sake of others who could be in my situation in the future: be explicit about this limitation in your README.

Thank you.

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.

6 participants