BREAKING: Android: Refactor so uimanager can depend on modules/core

[rigdern]

cc @astreet

The goal of this PR is to enable the buck module uimanager to depend on modules/core without introducing any dependency cycles.

PR #11008 relies on this PR. PR #11008 needs uimanager to depend on modules/core so that uimanager can fire events using RCTDeviceEventEmitter which is in modules/core.

Summary of Breaking Changes

This PR moved a number of classes and interfaces:

• com.facebook.react.modules.debug.DeveloperSettings -> com.facebook.react.modules.debug.interfaces.DeveloperSettings
• com.facebook.react.devsupport.DevOptionHandler -> com.facebook.react.devsupport.interfaces.DevOptionHandler
• com.facebook.react.devsupport.DevSupportManager -> com.facebook.react.devsupport.interfaces.DevSupportManager
• com.facebook.react.devsupport.DevServerHelper.PackagerStatusCallback -> com.facebook.react.devsupport.interfaces.PackagerStatusCallback
• The class com.facebook.react.devsupport.StackTraceHelper.StackFrame was renamed to StackFrameImpl. StackFrameImpl implements a new interface called com.facebook.react.devsupport.interfaces.StackFrame.
• com.facebook.react.uimanager.AppRegistry -> com.facebook.react.modules.appregistry.AppRegistry
• com.facebook.react.uimanager.ReactChoreographer -> com.facebook.react.modules.core.ReactChoreographer

Alternative Solution

One possible solution to this problem is to move RCTDeviceEventEmitter out of modules/core into its own buck module. However, I suspect that this would be a breaking change that would affect real apps. RCTDeviceEventEmitter would have to move into a different package and I suspect RCTDeviceEventEmitter is a somewhat common API in third-party code. Consequently, I decided not to go with this approach.

This Solution

The approach implemented in this PR involves moving around a number of classes/interfaces. I believe that these class/interfaces aren't commonly used in third-party code so it won't break most apps.

I will explain the refactoring I implemented in this PR in several steps.

Step 1: Create buck module modules/debug:interfaces

The purpose of this buck module is that later we're going to create a devsupport:interfaces module. devsupport:interfaces needs to be able to depend on DeveloperSettings without depending on all of modules/debug. If it depended on all of modules/debug, then we'd have this cycle:

uimanager -> modules/core -> devsupport:interfaces -> modules/debug -> uimanager

New modules/debug:interfaces module contains:

• DeveloperSettings

These modules depend on it:

• devsupport
• modules/debug
• react

Step 2: Remove modules/core's dependency on devsupport

This change addresses the following cycle:

uimanager -> modules/core -> devsupport -> modules/debug -> uimanager

We break this cycle by removing modules/core's dependency on devsupport. We pull the devsupport stuff that module/core depends on out into its own buck module, devsupport:interfaces.

Thus, the dependency chain becomes:

uimanager -> modules/core -> devsupport:interfaces -> modules/debug:interfaces

New devsupport:interfaces module contains:

• DevOptionHandler
• DevSupportManager
• PackagerStatusCallback
• StackFrame

The PackagerStatusCallback interface was extracted from the class DevServerHelper and moved into its own file. PackagerStatusCallback is in devsupport:interfaces and DevServerHelper remains in devsupport.

StackFrame is a new interface. There used to be a class called StackFrame but it has been renamed to StackFrameImpl and it implements StackFrame. StackFrame is in devsupport:interfaces and StackFrameImpl is in devsupport.

These modules depend on the new module:

• devsupport
• modules/core
• react

Step 3: Remove jstasks' dependency on uimanager

This change addresses the following cycle:

uimanager -> modules/core -> jstasks -> uimanager

We break this cycle by removing jstasks' dependency on uimanager. The only thing in uimanager that jstasks depends on is AppRegistry. We break this cycle by moving AppRegistry to its own buck module called appregistry.

Thus, the new chain becomes:

uimanager -> modules/core -> jstasks -> appregistry

New appregistry module contains:

• AppRegistry

These modules depend on it:

• jstasks
• react

Step 4: Remove modules/core's dependency on uimanager

This change addresses the following cycle:

uimanager -> modules/core -> uimanager

We break this cycle by removing modules/core's dependency on uimanager. The only thing in uimanager that modules/core depends on is ReactChoreographer. We move ReactChoreographer from uimanager to modules/core.

New modules that depend on modules/core:

• uimanager

Adam Comella
Microsoft Corp.

[astreet]

This looks good to me: can you title this "BREAKING: " and make it clear at the top of the description what classes people have been using that would be affected? I'll import afterwards (though I'll probably have to land manually on Monday)

Thanks!

[rigdern]

@astreet Thanks for the fast response time with the review. I updated the title and description as requested.

I have a general question related to breaking changes in Java code. This PR is breaking because I moved around some classes and interfaces. It seems to me that the public API surface that third-party modules can use is not clearly defined and third-party modules can just make use of any code that is marked public even if it wasn't intended for third-party code to consume. It seems like this makes changing RN fragile and many changes to RN's implementation details can be considered to be breaking.

Does my concern make sense or am I missing something? If my concern is legit, is there some way we could clearly define the RN API surface that is intended to be consumed by third-parties and generate a compiler error if third-party code attempts to use code that isn't exposed on that API surface?

[astreet]

@facebook-github-bot shipit

Thanks!

[astreet]

Looks like there might be some conflicts that need to be resolved

[astreet]

You're right that the surface area is not well defined: we've opted to expose a lot so people can make 3rd party modules that interact with the inner-workings of RN, but yeah, that makes them more likely to break. We do try to keep the actual JS API and standard native modules/view manager apis as stable as possible and consider changes there Big Deals (tm). Really, we probably don't need to call these breaking since the vast majority of people won't encounter problems from it, but I think it's nice to have it listed.

[rigdern]

@astreet I rebased and resolved the merge conflicts.

[astreet]

@facebook-github-bot shipit

[facebook-github-bot]

@astreet has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.