Unify and document our macOS diff tags

[Saadnajmi]

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

1. There's a mix of internal and public issue numbers, leading to inconsistent tags
2. "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.
3. 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:

• @harrieshin : "Brackets with words inside look like Objective C code and confuse me
• @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. // macOS and // [macOS don'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:

1. ifdef macOS only code

#if TARGET_OS_OSX
  ...macOS only code
#endif

1. ifdef iOS only code

#if !TARGET_OS_OSX
  ...iOS only code
#endif

1. 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

1. Which tag format should we choose: (A) or (B)
2. Should diff tags be on the same line as ifdefs?: (A) or (B)
3. 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)

[Saadnajmi]

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

# Single Line diff
// [macOS]

# Block Diff
...
// [macOS
... 
// ]macOS
...

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

D)
# Single Line diff
// [macOS-diff]

# Block Diff
...
// [macOS-diff
... 
// ]macOS-diff
...

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:

#if !TARGET_OS_OSX  // [macOS]
...ios code
#else // macOS [
... macos code
#endif // macOS ]

My current PR had changed this, and I'll change it back.

[Saadnajmi]

Followup comment by @lyzhan7

Maybe we can change the if/else defines to have macOS first so we don't have the negative?

#if TARGET_OS_OSX  // [macOS
... macos code
#else // macOS]
... ios code
#endif // [macOS]

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]

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.