Skip to content

Conversation

mganandraj
Copy link

@mganandraj mganandraj commented Feb 27, 2020

Please select one of the following

  • I am removing an existing difference between facebook/react-native and microsoft/react-native 👍
  • I am cherry-picking a change from Facebook's react-native into microsoft/react-native 👍
  • I am making a fix / change for the macOS implementation of react-native
  • I am making a change required for Microsoft usage of react-native

This change reverts almost all the android specific changes from this fork. All the reverted changes are now in the form of patch files which can be applied over the repo. We are using the standard unix diff/patch tools.

In order to apply the patches in lab where the pathing tools may not be available, we are using an embedded patcher written in Javascript. I've copied and adapted code from https://github.com/ds300/patch-package repo for patching the source files.

Almost all the processing is automated. The diff/patch files are created by comparing our fork with a clone of fb repo and running unix diff command on each files which are different. We later apply reverse patch on our fork to revert the differences. I've verified that "revese-patching + patching" the fork with the current set of patch files results in clean repo.

Focus areas to test

OFfice applications on Android platform needs to be tested to make sure that we don't regress anything.

Microsoft Reviewers: Open in CodeFlow

Copy link

@NickGerleman NickGerleman left a comment

Choose a reason for hiding this comment

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

ReactCommon looks good. Don't have enough context on other areas to give any meaningful feedback.

@alloy
Copy link
Member

alloy commented Feb 28, 2020

In order to apply the patches in lab where the pathing tools may not be available, we are using an embedded patcher written in Javascript. I've copied and adapted code from https://github.com/ds300/patch-package repo for patching the source files.

@mganandraj Meta question; can you elaborate on the changes you had to make to the tool and if those don’t make sense to send upstream?

@mganandraj
Copy link
Author

In order to apply the patches in lab where the pathing tools may not be available, we are using an embedded patcher written in Javascript. I've copied and adapted code from https://github.com/ds300/patch-package repo for patching the source files.

@mganandraj Meta question; can you elaborate on the changes you had to make to the tool and if those don’t make sense to send upstream?

One change was the treatment of new files. "ds300/patch-package" assumes that all patches are modifications to existing files. But, we decided to create patch files for new files too (using --unidirectional-new-file option to diff) so that we treat every difference as patch which can be applied irrespective of the type of difference (whether change, new, delete file). Essentially, we want to avoid any metadata over the patches. This decision was taken before we realized that we can't run standard "patch" tool in lab, and would need to use something like "ds300/patch-package"'s patching code. The change is fairly simply, to make sure the (empty) target directory/file exists before applying patches..
Another change was to ignore the target path in specified in the patch file, if the caller specifies an override target file.
Then, i've added a few logging to better debug.

@mganandraj
Copy link
Author

In order to apply the patches in lab where the pathing tools may not be available, we are using an embedded patcher written in Javascript. I've copied and adapted code from https://github.com/ds300/patch-package repo for patching the source files.

@mganandraj Meta question; can you elaborate on the changes you had to make to the tool and if those don’t make sense to send upstream?

One change was the treatment of new files. "ds300/patch-package" assumes that all patches are modifications to existing files. But, we decided to create patch files for new files too (using --unidirectional-new-file option to diff) so that we treat every difference as patch which can be applied irrespective of the type of difference (whether change, new, delete file). Essentially, we want to avoid any metadata over the patches. This decision was taken before we realized that we can't run standard "patch" tool in lab, and would need to use something like "ds300/patch-package"'s patching code. The change is fairly simply, to make sure the (empty) target directory/file exists before applying patches..
Another change was to ignore the target path in specified in the patch file, if the caller specifies an override target file.
Then, i've added a few logging to better debug.

Alloy, i'll work on pushing some of the changes once we settle on this PR. THanks!

…st. If this doesn't work then will look deeper
@tom-un
Copy link
Collaborator

tom-un commented Mar 4, 2020

Hi @mganandraj :
Regarding the Apple PR test failures:

  • There is a jest test failure because a snapshot file is no longer used. You can clean it up by running yarn test -u.
  • The ObjC class RCTCxxBridge.mm is failing to build because the second argument of initializeBridge() on facebook::react::Instance went away in this PR. So you need to delete line 599 in RCTCxxBridge.mm.

@ghost ghost removed the Needs: Author Feedback label Mar 18, 2020
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.

5 participants