rnx-kit icon indicating copy to clipboard operation
rnx-kit copied to clipboard

List of improvements for incubating package `patch-rnmacos`

Open kelset opened this issue 2 years ago • 1 comments

This issue wants to track a series of improvements that can be done for the draft package @rnx-kit/patch-rnmacos to get it to be is a better shape (and potentially graduate from draft into something that we'd like to share more broadly).

Here's the list:

  • [ ] add proper testing. Currently, there is none.
  • [ ] Some modules names are camelCase and some are snake_case. Make it consistent within the package (either one is fine).
  • [ ] the tool currently defaults to Windows values: it should be able to figure out what platform it is on and choose an appropriate set of executables.
  • [ ] change the --confirm true param into --write without a value.
  • [ ] change program.version("0.0.1"); in cli.ts to pick up the version from the package.json's field version
  • [ ] move the default inclusion and exclusion lists out of code and into a json or text file.
  • [ ] in diffRepos.ts, the code between the onHit and onMiss callbacks is nearly identical. Lift it out and share
  • [ ] remove commented out code if deemed useless
  • [ ] replace packages/draft-patch-rnmacos/src/file_compare.ts with hasha.fromFileSync() (from https://github.com/sindresorhus/hasha). It does the same thing, it's maintained by someone else, and it uses worker threads for faster perf.
  • [ ] make exclusion list be pre-processed to exclude ./ or .\ once, ahead of time. See for example ~L114 in packages/draft-patch-rnmacos/src/fs_utils.ts
  • [ ] Git can generate and apply file-level patches. Why not use that, vs writing our own impl of parsing and applying patch files? (see patch/apply.ts as an example of something that could be replaced potentially)
  • [ ] a few more improvements for patch/apply.ts:
    • [ ] change logic on L30 - from comment: "What about a check to see if the target exists? If it does, that seems like a problem.... by default, moveSync fails if the target exists already."
    • [ ] comment: The dryRun checks seem valuable in the non-dryrun case. Consider running them all the time, and then only doing the destructive ops when !dryRun.
    • [ ] L63 - from comment: "What about other mode changes, like read and write?"
  • [ ] logic in L176->L197 for packages/draft-patch-rnmacos/src/diffRepos.ts should be revised: If something is present in both inclusionList as well as exclusionList, you should probably fail the whole run.
  • [ ] add a new command update-patches, to attempt fixing a patch after the codebase has changed. See an attempt at implementation here: https://github.com/microsoft/react-native-macos/pull/864

Feel free to pick up any one of these bullet points separately - simply leave a comment here to let everyone know that you are working on it.


most of the items listed were originally reported here https://github.com/microsoft/rnx-kit/pull/1135

kelset avatar Mar 01 '22 11:03 kelset

  • [ ] patch tool has a number of default app paths in it. these should be replaced with parameterized values.
  • [ ] patch tool makes assumptions about running on Windows. it needs to handle cases where it is run on MacOS or Ubuntu (CI loops).

afoxman avatar Mar 01 '22 17:03 afoxman