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

[Feature] Reference a PR via URL to create Patch #15

Open
D1no opened this Issue Sep 11, 2017 · 5 comments

Comments

Projects
None yet
3 participants
@D1no

D1no commented Sep 11, 2017

The list of outstanding PRs in react-native and the react community is long. It would be great, if we could add links to those PRs i.e. inside the package json, so the PR is applied automatically.

As an interface we could simply add them to the package.json

Proposal

By simply applying .patch or .diff to a github PR, a full fledge patch is provided. This would make patching much simpler.

Try it out:
Pull Request: facebook/react-native#12807
Pull Request as Patch: https://github.com/facebook/react-native/pull/12807.patch

To manage this centrally, we could just use the package.json:

// pacakge.json
{
  "name": "name",
  "version": "1.0.0",
  "dependencies": {
    "react": "16.0.0-alpha.12",
    "react-native": "^0.47.2",
    "react-native-scandit": "^1.1.7",
    "styled-components": "^2.1.2",
    "patch-package": "X.XX"
  },

// New Property for patch-package
  "patch-package": {
    "react-native": [
      {
        // Link to PR. Most cases, all what is needed.
        "pullRequest": "https://github.com/facebook/react-native/pull/12807",
        // Optional. Default is head. Leads to: https://github.com/facebook/react-native/pull/12807/commits/1a44b86e4af6dbb62819348cd963ffa6a443f32e
        "commit": "1a44b86e4af6dbb62819348cd963ffa6a443f32e", 
        // Optional. Restrict to specific package version or version range
        "version": "^0.47.2",
        // Optional. In case the package to be patched is in a subfolder of the repository
        "fromBaseFolder": ".", 
        // Optional. In case the folder we want to patch is called differently in the npm distribution, due to some build script
        "targetBaseFolder": ".", 
        // Optional. Restrict the scope of the applied patch
        "restrictChangesToPathFromBaseFolder": [
          "ReactAndroid/src/main/java/com/facebook/react/uimanager",
          "ReactAndroid/src/main/java/com/facebook/react/views/webview"
        ] 
      },
      // next patch in array for react-native
      {} 
    ]
  },
// End new Property

The patch file follows all the commits in the subject line. So to limit a patch up until a specific point, simply truncate it after the last wanted SHA. I.e. sometimes PRs are merged so late, that they are adapted to a new published version we not yet use.

screen shot 2017-09-11 at 10 45 09

Relevance

patch-package is necessary. The maintainers can just not follow up. However, this is an even stronger incentive to create private hacks because patches are difficult to share. By allowing a "simple" reference to a PR, the community is incentivised to make at least a PR to the upstream package and reference that PR them self as an easier way of patching until it is merged.

Btw: This approach has the nice side effect, that testing PRs becomes dump easy for novices. Up on problems, people will report back/complain to the PR, enforcing quality and bringing less garbage PRs inside a repo!

Prior to patch package, we applied a few patches with the vanilla bash commands via a post install script. Not ideal.

And I said it before. Love your sharp mind in all this.

@D1no D1no changed the title from [Feature] Reference open PR to create Patch to [Feature] Reference a PR via URL to create Patch Sep 11, 2017

@ds300

This comment has been minimized.

Owner

ds300 commented Sep 11, 2017

This is a really really cool idea. I especially love:

By allowing a "simple" reference to a PR, the community is incentivised to make at least a PR to the upstream package and reference that PR them self as an easier way of patching until it is merged.

Unfortunately it wouldn't work for packages which have pre-publish compilation steps. I'll think about it some more and get back to you.

@D1no

This comment has been minimized.

D1no commented Sep 11, 2017

Dealing with build artefacts is of course an issue. And as a TypeScript advocate myself, I can see where you're coming from. However, in such cases a true fork should really be considered. The only alternative would be to run a real git workflow under the hood and do some crazy npm publishing to a local registry to fetch the data. I.e. with an .npmrc (https://docs.npmjs.com/files/npmrc). And that might not even cover all the cases, as many maintainers are unaware of prepublish hocks or don't use them to its full extend. Rather implementing own build scripts.

So that would be issues, that are even difficult to cover by just simply forking a project or cloning a repository.

Therefore I'd say there is just so much one can do with a patch and build artefacts are out of scope.

@ds300

This comment has been minimized.

Owner

ds300 commented Sep 19, 2017

Hey @D1no sorry for the delay in responding to this. I decided that I'd really like to support this idea and will test out some versions of it. There seems to be many edge cases and it will be a real challenge to figure out how to mitigate them and provide a good DX, but I feel that the community benefits would be worth the effort if done well.

Thanks for taking the time to share your idea. It wasn't on my radar.

@D1no

This comment has been minimized.

D1no commented Sep 20, 2017

Sure no Problem! If you create a feature PR on this, feel free to tag me. We (or I) don't directly need this — but the community surely does. Cheers!

@0x5e

This comment has been minimized.

0x5e commented Oct 12, 2017

Really good idea & cool project, I am using some old react-native related package, due to different gradle version or react-native version, there is always some patches I need to apply, if I wait the package owner to merge my PR and then publish to npm, maybe a month :-) Thanks again anyway!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment