Add applyPatches script and some basic patch documentation

[baparham]

In trying to apply patches for new versions, I found it helpful to be able to iterate a bit faster when adjusting the patches without having to run a full build each time. I also thought it might be helpful to add some basic documentation to show folks how to do this themselves. Hopefully this enables more hands to help keep this very useful library up to date.

Please let me know if you think anything can be improved. I appreciate the feedback.

[jesec]

Thanks for the PR, but I don't think this feature would be useful.

Developers work on the Node.js repository, not patches themselves. Patches are generated from a Git diff.

[baparham]

This addition also opens the door to providing a PR test suite that applies patches only without building to quickly inform the submitter if they've done something wrong, and to allow you to not have to ask if the patches apply cleanly.

Of course, it may make sense instead to just build the new binary too, test for clean patch application somehow, and run a basic hello world compiled pkg application to test that the new patches of node work as expected. This seems like it would drastically increase the ease for you to merge in patch updates from external contributors.

I'm going to reopen and see if that use case makes sense to you.

[baparham]

Nice, so apparently node10 patches don't apply cleanly, so says the new test 👍

[edit: I'll just drop support for 10 for now]

[baparham]

The new test is just a simple bash script that I put together to check the output of npm run applyPatches that was introduced in this PR, so I'm open to feedback on it's quality. I was focused more on the functionality just to see if it was feasible.

[robertsLando]

Cool, I like the new test you added. LGTM

[baparham]

Do we need @leerob to merge this in?

[robertsLando]

I can merge this but we need him for the release

[robertsLando]

Based on what I remember we planned to support also last EOF release (in this caase it would be nodejs 12). Could you restore that patch?

[baparham]

@robertsLando ah, ok. In not at my computer this morning, but I'll do this in an hour or two

[robertsLando]

Also I'm wondering why nodejs 10 patches are failing if we actually have binaries for them, could you investigate? Maybe the test script is getting some false negative? Would be good to don't drop any patch where possible, at least I know there are still someone using nodejs 8 patches...

[baparham]

Also I'm wondering why nodejs 10 patches are failing if we actually have binaries for them, could you investigate? Maybe the test script is getting some false negative? Would be good to don't drop any patch where possible, at least I know there are still someone using nodejs 8 patches...

It was that the patch applied but with a minor offset in one place (you can see it in the test output: https://github.com/vercel/pkg-fetch/actions/runs/4016498998/jobs/6899608207) so it's not "broken" it just breaks the rules for merging in new patches we've set for ourselves.

I'll fix the 10 patch instead of dropping the support, since as you say, people might still rely on 8, 10 and 12 from pkg

[baparham]

Reworded all the commits as well to better match the conventional commit style of the existing commits. Need a test for that someday :-)

[robertsLando]

Perfect! LGTM 🚀

[robertsLando]

@leerob Could we merge this?

Remember that new pkg-fetch releases needs a bit of attention. Minor/major releases need to also provide all the binaries required otherwise pkg would stop working (happened with 5.0.0) release and i don't remember if we stick a pkg-fetch version in its pkg.json after that

Also dunno if @baparham would also provide a way to automatically update the hashes and so on like he mentioned in a comment, AFAIK the releases steps where to wait for workflows to end and then calculate the sha of each file and commit them in a separeted commit and this was made by @jesec previously on pkg-fetch side

[leerob]

Thank you! Very helpful.