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

Proposal: Tooling for the Future of Forked Files #4104

Closed
NickGerleman opened this issue Feb 14, 2020 · 2 comments
Closed

Proposal: Tooling for the Future of Forked Files #4104

NickGerleman opened this issue Feb 14, 2020 · 2 comments

Comments

@NickGerleman
Copy link
Collaborator

NickGerleman commented Feb 14, 2020

Summary

While React Native Windows has moved off the microsoft/react-native fork, mechanisms still exist to modify upstream React Native code. Our C++ code is mostly clean of changes, but we still have significant changes to upstream JavaScript. These changes, and the mechanisms we use to apply them increase fragility and lead to slow merges. Some of our problems can be mitigated by investment in tooling.

Our Current System

Platform-specific JavaScript is a necessity. Metro (the React Native bundler) accomplishes this by allowing per-platform implementations of a JavaScript module. E.g. We could have Widget.android.js and Widget.ios.js, with Metro resolving the correct module if a code asks to import from ./Widget. React Native Windows makes extensive use of this capability, with 175 files shared between .windows, .win32, and .windesktop. Apart from providing platform-unique code, we abuse this system to override existing shared JavaScript. This accounts for 60% of overrides. Other overrides for non-shared JavaScript are often just copies of Android or iOS implementations with minor changes. These files are kept together in a structure that’s merged with React Native source code during the JavaScript build.

While changing code internal to component boundaries is inherently fragile, this system adds additional unnecessary pain:

  1. We cannot trivially determine what files provide Windows-unique code compared to changing upstream.
  2. It’s not possible to tell what has changed in a patch style override without manual diffing.
  3. If shared JS isn't available, it’s not clear if we need to merge with Android or iOS upstream changes.
  4. The system relies on manual intervention to catch and update files that have changed upstream.
  5. The process of patching shared code, which we want to heavily discourage, is the same as adding Windows-specific components, which is necessary.

Some of these pains can be eliminated by changing how we record and represent overrides.

Adding the Override Manifest

Overrides can be divided into three rough categories:

  • Patches: changes made to existing shared libraries. These should be heavily discouraged, due to their high upgrade cost and fragility. Some of these may be upstreamed, but many are to mitigate an incomplete API-set
  • Platform abstraction: changes that implement Windows-specific behavior only. Apart from changes to the platform abstraction API, these changes are resilient to upgrades and don't require merging.
  • Derived: changes that implement a Windows specific version of a component, but share logic with other components. E.g. we could take a copy of an Android component, and make changes so they work with Windows. Theses changes are candidates for better upstream platform abstraction, and often need merging.

These different categories have different ideal representations, and require different knowledge to achieve tool-based merging. Patches can be kept as a diff, derived overrides require knowing the file they derive from. Abstraction logic doesn't require much attention.

We can embed this extra information into a manifest, associating metadata with overrides. The addition of structured metadata opens new opportunities around merge tooling, version checking, and system enforced issue tracking.

{
    "overrides": [
      {
        "category": "platform",
        "file": "Libraries/Theme/Theming.win32.js"
      },
      {
        "category": "derived",
        "file": "Libraries/Alert/Alert.win32.js",
        "base": "Libraries/Alert/Alert.android.js",
        "baseHash": "7CE94894C507917A15E64AD503F8EF6A9CA2C8CE",
        "baseVersion": "0.61.5",
        "issue": 4587
      },
      {
        "category": "patch",
        "file": "Libraries/Core/Important.win32.js.patch",
        "base": "Libraries/Core/Important.win32.js",
        "baseHash": "B9E3583DDCB8A4E4F587999FA54B333E1CB10B8B",
        "baseVersion": "0.60.6",
        "issue": 5123
      }
    ]
  }

The manifest will initially contain the following:

Field Description
category Describes which of the above categories an override falls into. This can assist tooling to understand how to handle an override during merging.
file Path to a registered override
base File the override is based on. Used to let us know what file to look for as the original while merging.
baseHash SHA1 hash (or similar) of the base file at override creation time. This can be used to detect if an override is out of date during update.
baseVersion The package version at the time of patch creation. This lets tooling know where to find an original file when doing a three-way merge
issue An issue tracking the upstream integration or removal plan of an override

Override Validation and Generation in the JS Build

During yarn build we will copy override files to a shared directory cohabitated with React Native source. This build step can be expanded to work with the manifest, adding integrity checking and patching. A just task could be added to yarn lint to check all files in src against the manifest, verifying the override has metadata, and that its base file hasn't changed since creation. This task should create an actionable error pointing to the faulty or outdated override. Doing this check during the lint phase allows yarn build to work with unregistered overrides during local development.

Patching can also be implemented as a simple just task during build time, using the patch file to create an override in the target file structure.

The Override CLI

CLI based tools should be added to manage overrides and upgrades. We need a few commands for different scenarios:

yarn override --add

Present a wizard style series of prompts to the developer to add an override to the manifest. E.g. asking type, asking for the creation of a Github issue with a plan, and doing generation of hashes or patch files.

yarn override --verify

Validate the the all files are correctly described in the manifest.

yarn override --remove

Removes an override from the manifest 👍

yarn override --patchify and yarn override --unpatchify

Convert a file to or from a diff based representation. This is needed to allow editing of a file while still storing changes as diffs. Since we have full knowledge of original vs new sources, this step can programmatically add comments around changed areas.

yarn override --update and yarn override --finalize

Used to upadte patches to a new base version. The yarn override --update step does two things

  1. Converts outdated patch files to full files, mechanically adding comments around diffs in the full file representation
  2. Stages the generated file (or current derived file), then replaces it with the new version of the base file

This allows simple three-way merging using standard Git tools (e.g. VSCode) to compare current vs staged. Eg:

image

Once everything is updated, you can run yarn override --finalize to convert files back to patches and update the manifest.

@ghost ghost added the Needs: Triage 🔍 New issue that needs to be reviewed by the issue management team (label applied by bot) label Feb 14, 2020
@NickGerleman NickGerleman removed the Needs: Triage 🔍 New issue that needs to be reviewed by the issue management team (label applied by bot) label Feb 14, 2020
@NickGerleman NickGerleman self-assigned this Feb 14, 2020
@alloy
Copy link
Member

alloy commented Feb 14, 2020

Somewhat off-topic, but just want to mention the existence of https://github.com/ds300/patch-package, which is a fairly popular JS tool to patch packages in node_modules post-install. While your proposal needs more fine-grained features, it might still be a helpful resource or, if it can be built upon, it might improve the generic tooling available to the JS community as a whole.

@kmelmon
Copy link
Contributor

kmelmon commented Feb 18, 2020

Note that @jonthysell is going to help drive the resolution of react-native-community/discussions-and-proposals#182, which is related - whatever solution we come up with will need to work with the tooling proposed here.

NickGerleman added a commit to NickGerleman/react-native-windows that referenced this issue Feb 21, 2020
Add a foundation for new override tooling described in microsoft#4104. This includes:

- Build scripts, lint scripts, config files, etc
- Logic for parsing and checking validity of an override manifest
- Unit tests for override manifest logic
- Abstractions to allow fetching React Native files of arbtrary versions

A lot of this is foundational. The override logic has been well-tested,
and the Git logic has been manually tested, but we don't have much
end-to-end set up yet.
ghost pushed a commit that referenced this issue Feb 22, 2020
* Initial commit of override tooling

Add a foundation for new override tooling described in #4104. This includes:

- Build scripts, lint scripts, config files, etc
- Logic for parsing and checking validity of an override manifest
- Unit tests for override manifest logic
- Abstractions to allow fetching React Native files of arbtrary versions

A lot of this is foundational. The override logic has been well-tested,
and the Git logic has been manually tested, but we don't have much
end-to-end set up yet.

* Address comments and deuplicate lockfile

* Add more dependencies for WebDriverIO

We hardcode an old version of WebderiverIO beacuse of #3019. These seem to have loose dependency requirements, because the change to deuplicate packages broke this (see webdriverio/webdriverio#4104). Hardcode resolutions in E2ETest for existing versions of wdio packages in the meantime.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants