Patched dependencies are breaking GCP deployments using pnpm about isolate-package HOT 9 CLOSED

Thanks for the example that will be an easy fix 👍

I think I will make it the default, with an option to turn it off. I want most users to be able to have zero-config, and it seems more common to want to strip the patches than to preserve them.

Yes, each function is it's own package to keep them small. I'm using GCP functions directly rather than firebase, and using terraform to deploy the resources. I use turborepo to generate function template folders and isolate-package is part of that now :)

Interesting. Terraform is something that was on my to-study list and I didn't know turborepo could generate template folders. I think I still would struggle a bit navigating that many packages, but it's good to know about the approach.

Thanks for the kind words. I will try to implement this soon.

from isolate-package.

@erik-lissen Thanks for reporting. I have no experience with patched dependencies. Could you share the lockfile maybe, so I get a better idea where they need to be stripped?

Do I understand correctly that there is no use-case for having patched dependencies as part of an isolated package lockfile, and therefore they can always be removed?

from isolate-package.

I read a bit about patching dependencies and why it is done.

As I understand it, it would be essential to keep the patch part of the isolated package, otherwise the fix will be undone in the deployed code. So I suspect that a solution could be:

• Make sure the patch file ends up in the output bundle
• Adapt the lockfile so that it points to the correct patch filepath.

What do you think?

If this feature is important to you, maybe try to submit a PR, because I do not think I will be working on this very shortly. I have too much on my plate already and this seems like a very rare use-case.

Alternatively, you could try to force output to generate an NPM lockfile with the forceNpm configuration option. Maybe that solves something, but I doubt it because the patch would probably still need to be copied to the isolate output.

from isolate-package.

(in my case, this patch is from my frontend code which has no business breaking my backend)

Forgive me, now I understand what you meant with this line.

So even though patches could be essential for deployed code, we need the option to strip them from the lockfile because the patches might not apply to the code that is being isolated.

In that case, I think the fix could be easy, and I'm willing to look at it if you show me your lockfile.

Also, my suggestion for forceNpm then seems like a valid thing to try in the meantime. The lockfile adaption logic is different between pnpm and npm. In case of pnpm the lockfile is adapted using my own logic, in the case of npm the lockfile is regenerated by a library that is part of npm itself.

Note that your codebase can still use pnpm, it's just that the option generates an npm compatible isolate.

from isolate-package.

So even though patches could be essential for deployed code, we need the option to strip them from the lockfile because the patches might not apply to the code that is being isolated.

Yes, it's trying to patch something that is not in my isolated package.

In that case, I think the fix could be easy, and I'm willing to look at it if you show me your lockfile.

That would be great - it's a minor inconvenience not to have it, not a huge deal. Right now, we don't have any patches for the backend code, so I just run a node script to strip out the patched deps, job done. But if I had some patches on my functions it would be a lot more problematic.

Also, my suggestion for forceNpm then seems like a valid thing to try in the meantime. The lockfile adaption logic is different between pnpm and npm. In case of pnpm the lockfile is adapted using my own logic, in the case of npm the lockfile is regenerated by a library that is part of npm itself.

I tried forceNpm, but it has another problem - I'm using turborepo to isolate many packages all at once - because of how the node_modules folder is moved in-out of its place with forceNpm, it can only be done one-by-one which will take too long (I have 35 functions at the moment) - as the node_modules folder gets deleted while something else is trying to use it.

from isolate-package.

I tried forceNpm, but it has another problem - I'm using turborepo to isolate many packages all at once - because of how the node_modules folder is moved in-out of its place with forceNpm, it can only be done one-by-one which will take too long (I have 35 functions at the moment) - as the node_modules folder gets deleted while something else is trying to use it.

Ah right, that's an unfortunate side effect. Do you create a monorepo package for each cloud function you deploy? Is that to minimize cold-start times?

I'm just curious, because I have 30+ functions in my project as well, but they are only spread out over a few packages, and I never really considered breaking it up into one per package.

I think that patching dependencies, because it seems often related to using bleeding-edge libs, is more likely to be done on the frontend than backend. So I assume stripping it from the lockfile is sufficient for 99.9% of users. I'm not aware of any client-side deployments requiring an isolated package.

from isolate-package.

Ah right, that's an unfortunate side effect. Do you create a monorepo package for each cloud function you deploy? Is that to minimize cold-start times?

Yes, each function is it's own package to keep them small. I'm using GCP functions directly rather than firebase, and using terraform to deploy the resources. I use turborepo to generate function template folders and isolate-package is part of that now :)

Great package btw, I solved this myself previously with my own convoluted cli tool, but now that I moved to using turborepo your package is a huge help!

I think that patching dependencies, because it seems often related to using bleeding-edge libs, is more likely to be done on the frontend than backend. So I assume stripping it from the lockfile is sufficient for 99.9% of users. I'm not aware of any client-side deployments requiring an isolated package.

Yes, I think I agree with that statement. Just stripping all of it is probably sufficient. Otherwise, you'd have to check for any patches, go through the dependencies and check if any of those are included in the current package and strip the ones out that are not. I haven't looked at your code too much so I don't know if that's hard or not

Right now I'm just doing this for all my functions:

"scripts": {
    "isolate": "isolate && pnpm run removePatched",
    "removePatched": "node -e \"const fs = require('fs'), yaml = require('js-yaml'); let yamlData = yaml.load(fs.readFileSync('./isolated/pnpm-lock.yaml', 'utf8')); delete yamlData.patchedDependencies; fs.writeFileSync('./isolated/pnpm-lock.yaml', yaml.dump(yamlData), 'utf8');\""
  },

I would suggest just adding an option like stripPatchedDependencies that does that ⬆️

from isolate-package.

@erik-lissen I have released 1.10.0-1 under @next so could you give it a try and let me know if it works for you? I didn't test it myself, but I think this should work...

from isolate-package.

Should be fixed by 1.10.0, although I frankly didn't test it :)

from isolate-package.