Repository navigation
Unify and document our macOS diff tags #1579
Description
Purpose
I would like to standardize on our format for diff tags in React Native macOS. Diff tags are how we track the code diffs we have added on top of React Native to get React Native macOS compiling and working for our products. Overtime, we've shifted the format of these tags, which has resulted in less cleanliness and confusion around their purpose.
Current state
# Old single line comments, with an internal issue number
// TODO(OSS Candidate ISS#2710739)
# Current single line comments, with a public Github issue
// TODO(macOS GH#774)
# Blocks
...
// [TODO(macOS GH#774)
... code we added
// ]TODO(macOS GH#774)
...
Problems with current state
- There's a mix of internal and public issue numbers, leading to inconsistent tags
- "TODO" was because we thought one day we would upstream all of our diffs. That will probably never happen, so it gives the wrong impression.
- It's not obvious that
TODO(macOS GH#774)means Track macOS specific changes that differ from upstream #774 .
Proposal
Let's do something more similar to React Native Windows, which has tags that look like this:
# Single Line diff
// Windows
# Block Diff
...
// [Windows
...
// Windows]
...
The current format in my PR (#1392 ) looks like:
# Single Line diff
// macOS
# Block Diff
...
// [macOS
...
// macOS]
...
Some feedback I got:
- Harrie Shin (@harrieshin) : "Brackets with words inside look like Objective C code and confuse me
- Adam Gleitman (@amgleitman) : "It would be nice to have one phrase we can easily Cmd+Shift+F search the entire repo for all of our diff tags.
// macOSand// [macOSdon't allow me to do that."
Better format
Given the feedback, I came up with two new formats that satisfy the feedback I got.
A) Brackets before comment with a space
# Single Line diff
// [ macOS ]
# Block Diff
...
// [ macOS
...
// ] macOS
...
# Inline diff (new format)
/* this code adds an RCTUIView [ macOS ] to the view tree */
B) Brackets after the comment with a space
# Single Line diff
// macOS
# Block Diff
...
// macOS [
...
// macOS ]
...
# Inline diff (new format)
/* this code adds an RCTUIView [// macOS] to the view tree */
Bonus Section - ifdef blocks
Oftentimes we add a few types of ifdef blocks to React Native's iOS code:
- ifdef macOS only code
#if TARGET_OS_OSX
...macOS only code
#endif
- ifdef iOS only code
#if !TARGET_OS_OSX
...iOS only code
#endif
- ifdef between an iOS and macOS block
#if TARGET_OS_OSX
...macOS only code
#else
...original iOS block
#endif
Should tags be on the same line?
A) Same line
#if TARGET_OS_OSX // [ macOS
...macOS only code
#endif // ] macOS
B) Different line
// [ macOS
#if TARGET_OS_OSX
...macOS only code
#endif
// ] macOS
Should iOS ifdef blocks be single line or block tags?
A) Two single line tags
#if !TARGET_OS_OSX // [ macOS ]
...iOS only code
#endif // [ macOS ]
Rationale: A block makes it look like we're adding this code when we aren't. This is the current state.
B) Block tag
#if !TARGET_OS_OSX // [ macOS
...iOS only code
#endif // ] macOS
Rationale: The two ifdef tags do correspond to each other, even if the code inside isn't our addition
Final Poll
- Which tag format should we choose: (A) or (B)
- Should diff tags be on the same line as ifdefs?: (A) or (B)
- Should iOS blocks have block tags or two single line tags: (A) or (B)
(In a bit of cheating, I made my preference for each question the first option, AKA (a)
Activity
Saadnajmi commentedon Dec 13, 2022
Talked offline with my team, and came up with a few answers and new proposals:
Which tag format should we choose?
New options, C and D
C)
Let's just remove the space from option A. It should be clear in IDE's that this is not objective-c code
D)
Option C but let's also change it to "macOS-diff" to make it more clear it's a diff and not obj-c code
(I don't like this options as we lose parity with React Native Windows, but see the benefits
Should diff tags be on the same line as ifdefs?
We'll choose (A), with the exception of if the diff has an explanation to go with it. Then we can move it to a new line.
Should iOS blocks have block tags or two single line tags?
We'll choose (A). Similarly, if/else defines will need to be kept to this format:
My current PR had changed this, and I'll change it back.
Saadnajmi commentedon Dec 13, 2022
Followup comment by Lynn Zhang (@lyzhan7)
Maybe we can change the if/else defines to have macOS first so we don't have the negative?
My concern with this is it might confuse git a bit more having the additive code first rather than after the iOS code. I can try an experiment in a followup PR.
shwanton commentedon Dec 14, 2022
I like option C since it follows the pattern on windows.
For whether they should be on the same line, I think we should go with the version that stays similar to what we already do to reduce churn in future PR's. Same for ordering of macos/ios.