Fabric: Use ComponentDescriptorParameters as an actual parameter of ComponentDescriptor constructor

shergin · facebook-github-bot · commit a09ab5369236 · 2019-12-22T22:22:00.000-08:00
Summary:
This diff changes the signature of ComponentDescriptor constructor to make it simpler and easier to support: now all arguments are passed via struct that contains all these arguments as fields.

Now the ComponentDescriptor constructor accepts three arguments one of which is optional. This causes some confusion and the possibility of bugs in all subclasses that needs to implement a custom constructor. Mostly because in every case we need to ensure that the constructor:
 * Accepts and pass down all parameters/arguments;
 * Accepts the right types of those parameters (shared vs weak pointers, references vs values).
 * Accepts all thee arguments and pass them (including flavor!). We failed this point several times.

Overal that makes the code simpler and allows changing the set of parameters relatively easy. (There is no plan for it!)
Look at the LOC balance: less code!

Changelog: [INTERNAL]

Reviewed By: sammy-SC

Differential Revision: D18548173

fbshipit-source-id: 5d038b135e004f6c054026b3235ed57db99c086d
diff --git a/ReactCommon/fabric/components/image/ImageComponentDescriptor.h b/ReactCommon/fabric/components/image/ImageComponentDescriptor.h
@@ -21,12 +21,9 @@ namespace react {
 class ImageComponentDescriptor final
     : public ConcreteComponentDescriptor<ImageShadowNode> {
  public:
-  ImageComponentDescriptor(
-      EventDispatcher::Weak eventDispatcher,
-      ContextContainer::Shared const &contextContainer,
-      ComponentDescriptor::Flavor const &flavor = {})
-      : ConcreteComponentDescriptor(eventDispatcher, contextContainer, flavor),
-        imageManager_(std::make_shared<ImageManager>(contextContainer)){};
+  ImageComponentDescriptor(ComponentDescriptorParameters const &parameters)
+      : ConcreteComponentDescriptor(parameters),
+        imageManager_(std::make_shared<ImageManager>(contextContainer_)){};
 
   void adopt(UnsharedShadowNode shadowNode) const override {
     ConcreteComponentDescriptor::adopt(shadowNode);
diff --git a/ReactCommon/fabric/components/legacyviewmanagerinterop/LegacyViewManagerInteropComponentDescriptor.h b/ReactCommon/fabric/components/legacyviewmanagerinterop/LegacyViewManagerInteropComponentDescriptor.h
@@ -19,9 +19,7 @@ class LegacyViewManagerInteropComponentDescriptor final
   using ConcreteComponentDescriptor::ConcreteComponentDescriptor;
 
   LegacyViewManagerInteropComponentDescriptor(
-      EventDispatcher::Weak const &eventDispatcher,
-      ContextContainer::Shared const &contextContainer = {},
-      ComponentDescriptor::Flavor const &flavor = {});
+      ComponentDescriptorParameters const &parameters);
   /*
    * Returns `name` and `handle` based on a `flavor`, not on static data from
    * `LegacyViewManagerInteropShadowNode`.
diff --git a/ReactCommon/fabric/components/legacyviewmanagerinterop/LegacyViewManagerInteropComponentDescriptor.mm b/ReactCommon/fabric/components/legacyviewmanagerinterop/LegacyViewManagerInteropComponentDescriptor.mm
@@ -52,32 +52,27 @@
 }
 
 LegacyViewManagerInteropComponentDescriptor::LegacyViewManagerInteropComponentDescriptor(
-    EventDispatcher::Weak const &eventDispatcher,
-    ContextContainer::Shared const &contextContainer,
-    ComponentDescriptor::Flavor const &flavor)
-    : ConcreteComponentDescriptor(eventDispatcher, contextContainer, flavor),
-      _coordinator(constructCoordinator(contextContainer, flavor))
+    ComponentDescriptorParameters const &parameters)
+    : ConcreteComponentDescriptor(parameters), _coordinator(constructCoordinator(contextContainer_, flavor_))
 {
 }
 
-ComponentHandle
-LegacyViewManagerInteropComponentDescriptor::getComponentHandle() const {
+ComponentHandle LegacyViewManagerInteropComponentDescriptor::getComponentHandle() const
+{
   return reinterpret_cast<ComponentHandle>(getComponentName());
 }
 
-ComponentName LegacyViewManagerInteropComponentDescriptor::getComponentName()
-    const {
+ComponentName LegacyViewManagerInteropComponentDescriptor::getComponentName() const
+{
   return std::static_pointer_cast<std::string const>(this->flavor_)->c_str();
 }
 
-void LegacyViewManagerInteropComponentDescriptor::adopt(
-    ShadowNode::Unshared shadowNode) const {
+void LegacyViewManagerInteropComponentDescriptor::adopt(ShadowNode::Unshared shadowNode) const
+{
   ConcreteComponentDescriptor::adopt(shadowNode);
 
-  assert(std::dynamic_pointer_cast<LegacyViewManagerInteropShadowNode>(
-      shadowNode));
-  auto legacyViewManagerInteropShadowNode =
-      std::static_pointer_cast<LegacyViewManagerInteropShadowNode>(shadowNode);
+  assert(std::dynamic_pointer_cast<LegacyViewManagerInteropShadowNode>(shadowNode));
+  auto legacyViewManagerInteropShadowNode = std::static_pointer_cast<LegacyViewManagerInteropShadowNode>(shadowNode);
 
   auto state = LegacyViewManagerInteropState{};
   state.coordinator = _coordinator;
diff --git a/ReactCommon/fabric/components/modal/ModalHostViewComponentDescriptor.h b/ReactCommon/fabric/components/modal/ModalHostViewComponentDescriptor.h
@@ -21,16 +21,7 @@ namespace react {
 class ModalHostViewComponentDescriptor final
     : public ConcreteComponentDescriptor<ModalHostViewShadowNode> {
  public:
-#ifdef ANDROID
-  ModalHostViewComponentDescriptor(
-      EventDispatcher::Weak eventDispatcher,
-      ContextContainer::Shared const &contextContainer,
-      ComponentDescriptor::Flavor const &flavor = {})
-      : ConcreteComponentDescriptor(eventDispatcher, contextContainer, flavor) {
-  }
-#else
   using ConcreteComponentDescriptor::ConcreteComponentDescriptor;
-#endif
 
   void adopt(UnsharedShadowNode shadowNode) const override {
     assert(std::dynamic_pointer_cast<ModalHostViewShadowNode>(shadowNode));
diff --git a/ReactCommon/fabric/components/slider/SliderComponentDescriptor.h b/ReactCommon/fabric/components/slider/SliderComponentDescriptor.h
@@ -20,15 +20,12 @@ namespace react {
 class SliderComponentDescriptor final
     : public ConcreteComponentDescriptor<SliderShadowNode> {
  public:
-  SliderComponentDescriptor(
-      EventDispatcher::Weak eventDispatcher,
-      ContextContainer::Shared const &contextContainer,
-      ComponentDescriptor::Flavor const &flavor = {})
-      : ConcreteComponentDescriptor(eventDispatcher, contextContainer, flavor),
-        imageManager_(std::make_shared<ImageManager>(contextContainer)),
+  SliderComponentDescriptor(ComponentDescriptorParameters const &parameters)
+      : ConcreteComponentDescriptor(parameters),
+        imageManager_(std::make_shared<ImageManager>(contextContainer_)),
         measurementsManager_(
             SliderMeasurementsManager::shouldMeasureSlider()
-                ? std::make_shared<SliderMeasurementsManager>(contextContainer)
+                ? std::make_shared<SliderMeasurementsManager>(contextContainer_)
                 : nullptr) {}
 
   void adopt(UnsharedShadowNode shadowNode) const override {
diff --git a/ReactCommon/fabric/components/switch/androidswitch/AndroidSwitchComponentDescriptor.h b/ReactCommon/fabric/components/switch/androidswitch/AndroidSwitchComponentDescriptor.h
@@ -22,12 +22,10 @@ class AndroidSwitchComponentDescriptor final
     : public ConcreteComponentDescriptor<AndroidSwitchShadowNode> {
  public:
   AndroidSwitchComponentDescriptor(
-      EventDispatcher::Weak eventDispatcher,
-      ContextContainer::Shared const &contextContainer,
-      ComponentDescriptor::Flavor const &flavor = {})
-      : ConcreteComponentDescriptor(eventDispatcher, contextContainer, flavor),
+      ComponentDescriptorParameters const &parameters)
+      : ConcreteComponentDescriptor(parameters),
         measurementsManager_(std::make_shared<AndroidSwitchMeasurementsManager>(
-            contextContainer)) {}
+            contextContainer_)) {}
 
   void adopt(UnsharedShadowNode shadowNode) const override {
     ConcreteComponentDescriptor::adopt(shadowNode);
diff --git a/ReactCommon/fabric/components/text/paragraph/ParagraphComponentDescriptor.h b/ReactCommon/fabric/components/text/paragraph/ParagraphComponentDescriptor.h
@@ -23,17 +23,11 @@ namespace react {
 class ParagraphComponentDescriptor final
     : public ConcreteComponentDescriptor<ParagraphShadowNode> {
  public:
-  ParagraphComponentDescriptor(
-      EventDispatcher::Weak eventDispatcher,
-      ContextContainer::Shared const &contextContainer,
-      ComponentDescriptor::Flavor const &flavor = {})
-      : ConcreteComponentDescriptor<ParagraphShadowNode>(
-            eventDispatcher,
-            contextContainer,
-            flavor) {
+  ParagraphComponentDescriptor(ComponentDescriptorParameters const &parameters)
+      : ConcreteComponentDescriptor<ParagraphShadowNode>(parameters) {
     // Every single `ParagraphShadowNode` will have a reference to
     // a shared `TextLayoutManager`.
-    textLayoutManager_ = std::make_shared<TextLayoutManager>(contextContainer);
+    textLayoutManager_ = std::make_shared<TextLayoutManager>(contextContainer_);
   }
 
  protected:
diff --git a/ReactCommon/fabric/components/textinput/androidtextinput/AndroidTextInputComponentDescriptor.h b/ReactCommon/fabric/components/textinput/androidtextinput/AndroidTextInputComponentDescriptor.h
@@ -20,16 +20,11 @@ class AndroidTextInputComponentDescriptor final
     : public ConcreteComponentDescriptor<AndroidTextInputShadowNode> {
  public:
   AndroidTextInputComponentDescriptor(
-      EventDispatcher::Weak eventDispatcher,
-      const ContextContainer::Shared &contextContainer,
-      ComponentDescriptor::Flavor const &flavor = {})
-      : ConcreteComponentDescriptor<AndroidTextInputShadowNode>(
-            eventDispatcher,
-            contextContainer,
-            flavor) {
+      ComponentDescriptorParameters const &parameters)
+      : ConcreteComponentDescriptor<AndroidTextInputShadowNode>(parameters) {
     // Every single `AndroidTextInputShadowNode` will have a reference to
     // a shared `TextLayoutManager`.
-    textLayoutManager_ = std::make_shared<TextLayoutManager>(contextContainer);
+    textLayoutManager_ = std::make_shared<TextLayoutManager>(contextContainer_);
   }
 
  protected:
diff --git a/ReactCommon/fabric/core/componentdescriptor/ComponentDescriptor.cpp b/ReactCommon/fabric/core/componentdescriptor/ComponentDescriptor.cpp
@@ -11,12 +11,10 @@ namespace facebook {
 namespace react {
 
 ComponentDescriptor::ComponentDescriptor(
-    EventDispatcher::Weak const &eventDispatcher,
-    ContextContainer::Shared const &contextContainer,
-    ComponentDescriptor::Flavor const &flavor)
-    : eventDispatcher_(eventDispatcher),
-      contextContainer_(contextContainer),
-      flavor_(flavor) {}
+    ComponentDescriptorParameters const &parameters)
+    : eventDispatcher_(parameters.eventDispatcher),
+      contextContainer_(parameters.contextContainer),
+      flavor_(parameters.flavor) {}
 
 ContextContainer::Shared const &ComponentDescriptor::getContextContainer()
     const {
diff --git a/ReactCommon/fabric/core/componentdescriptor/ComponentDescriptor.h b/ReactCommon/fabric/core/componentdescriptor/ComponentDescriptor.h
@@ -18,6 +18,7 @@
 namespace facebook {
 namespace react {
 
+class ComponentDescriptorParameters;
 class ComponentDescriptor;
 
 using SharedComponentDescriptor = std::shared_ptr<ComponentDescriptor const>;
@@ -44,10 +45,7 @@ class ComponentDescriptor {
    */
   using Flavor = std::shared_ptr<void const>;
 
-  ComponentDescriptor(
-      EventDispatcher::Weak const &eventDispatcher,
-      ContextContainer::Shared const &contextContainer,
-      ComponentDescriptor::Flavor const &flavor);
+  ComponentDescriptor(ComponentDescriptorParameters const &parameters);
 
   virtual ~ComponentDescriptor() = default;
 
@@ -136,5 +134,16 @@ class ComponentDescriptor {
   Flavor flavor_;
 };
 
+/*
+ * Represents a collection of arguments that sufficient to construct a
+ * `ComponentDescriptor`.
+ */
+class ComponentDescriptorParameters {
+ public:
+  EventDispatcher::Weak eventDispatcher;
+  ContextContainer::Shared contextContainer;
+  ComponentDescriptor::Flavor flavor;
+};
+
 } // namespace react
 } // namespace facebook
diff --git a/ReactCommon/fabric/core/componentdescriptor/ConcreteComponentDescriptor.h b/ReactCommon/fabric/core/componentdescriptor/ConcreteComponentDescriptor.h
@@ -45,11 +45,8 @@ class ConcreteComponentDescriptor : public ComponentDescriptor {
   using ConcreteState = typename ShadowNodeT::ConcreteState;
   using ConcreteStateData = typename ShadowNodeT::ConcreteState::Data;
 
-  ConcreteComponentDescriptor(
-      EventDispatcher::Weak const &eventDispatcher,
-      ContextContainer::Shared const &contextContainer = {},
-      ComponentDescriptor::Flavor const &flavor = {})
-      : ComponentDescriptor(eventDispatcher, contextContainer, flavor) {
+  ConcreteComponentDescriptor(ComponentDescriptorParameters const &parameters)
+      : ComponentDescriptor(parameters) {
     rawPropsParser_.prepare<ConcreteProps>();
   }
 
diff --git a/ReactCommon/fabric/core/tests/ComponentDescriptorTest.cpp b/ReactCommon/fabric/core/tests/ComponentDescriptorTest.cpp
@@ -14,7 +14,8 @@ using namespace facebook::react;
 TEST(ComponentDescriptorTest, createShadowNode) {
   auto eventDispatcher = std::shared_ptr<EventDispatcher const>();
   SharedComponentDescriptor descriptor =
-      std::make_shared<TestComponentDescriptor>(eventDispatcher);
+      std::make_shared<TestComponentDescriptor>(
+          ComponentDescriptorParameters{eventDispatcher, nullptr, nullptr});
 
   ASSERT_EQ(descriptor->getComponentHandle(), TestShadowNode::Handle());
   ASSERT_STREQ(descriptor->getComponentName(), TestShadowNode::Name());
@@ -44,7 +45,8 @@ TEST(ComponentDescriptorTest, createShadowNode) {
 TEST(ComponentDescriptorTest, cloneShadowNode) {
   auto eventDispatcher = std::shared_ptr<EventDispatcher const>();
   SharedComponentDescriptor descriptor =
-      std::make_shared<TestComponentDescriptor>(eventDispatcher);
+      std::make_shared<TestComponentDescriptor>(
+          ComponentDescriptorParameters{eventDispatcher, nullptr, nullptr});
 
   const auto &raw = RawProps(folly::dynamic::object("nativeID", "abc"));
   SharedProps props = descriptor->cloneProps(nullptr, raw);
@@ -68,7 +70,8 @@ TEST(ComponentDescriptorTest, cloneShadowNode) {
 TEST(ComponentDescriptorTest, appendChild) {
   auto eventDispatcher = std::shared_ptr<EventDispatcher const>();
   SharedComponentDescriptor descriptor =
-      std::make_shared<TestComponentDescriptor>(eventDispatcher);
+      std::make_shared<TestComponentDescriptor>(
+          ComponentDescriptorParameters{eventDispatcher, nullptr, nullptr});
 
   const auto &raw = RawProps(folly::dynamic::object("nativeID", "abc"));
   SharedProps props = descriptor->cloneProps(nullptr, raw);
diff --git a/ReactCommon/fabric/core/tests/ShadowNodeTest.cpp b/ReactCommon/fabric/core/tests/ShadowNodeTest.cpp
@@ -17,7 +17,8 @@ using namespace facebook::react;
 
 TEST(ShadowNodeTest, handleShadowNodeCreation) {
   auto eventDispatcher = std::shared_ptr<EventDispatcher const>();
-  auto componentDescriptor = TestComponentDescriptor(eventDispatcher);
+  auto componentDescriptor = TestComponentDescriptor(
+      ComponentDescriptorParameters{eventDispatcher, nullptr, nullptr});
   auto family = std::make_shared<ShadowNodeFamily>(
       ShadowNodeFamilyFragment{
           /* .tag = */ 9,
@@ -47,7 +48,8 @@ TEST(ShadowNodeTest, handleShadowNodeCreation) {
 
 TEST(ShadowNodeTest, handleShadowNodeSimpleCloning) {
   auto eventDispatcher = std::shared_ptr<EventDispatcher const>();
-  auto componentDescriptor = TestComponentDescriptor(eventDispatcher);
+  auto componentDescriptor = TestComponentDescriptor(
+      ComponentDescriptorParameters{eventDispatcher, nullptr, nullptr});
   auto family = std::make_shared<ShadowNodeFamily>(
       ShadowNodeFamilyFragment{
           /* .tag = */ 9,
@@ -72,7 +74,8 @@ TEST(ShadowNodeTest, handleShadowNodeSimpleCloning) {
 
 TEST(ShadowNodeTest, handleShadowNodeMutation) {
   auto eventDispatcher = std::shared_ptr<EventDispatcher const>();
-  auto componentDescriptor = TestComponentDescriptor(eventDispatcher);
+  auto componentDescriptor = TestComponentDescriptor(
+      ComponentDescriptorParameters{eventDispatcher, nullptr, nullptr});
   auto family1 = std::make_shared<ShadowNodeFamily>(
       ShadowNodeFamilyFragment{
           /* .tag = */ 1,
@@ -147,7 +150,8 @@ TEST(ShadowNodeTest, handleShadowNodeMutation) {
 
 TEST(ShadowNodeTest, handleCloneFunction) {
   auto eventDispatcher = std::shared_ptr<EventDispatcher const>();
-  auto componentDescriptor = TestComponentDescriptor(eventDispatcher);
+  auto componentDescriptor = TestComponentDescriptor(
+      ComponentDescriptorParameters{eventDispatcher, nullptr, nullptr});
   auto family = std::make_shared<ShadowNodeFamily>(
       ShadowNodeFamilyFragment{
           /* .tag = */ 9,
@@ -181,7 +185,8 @@ TEST(ShadowNodeTest, handleCloneFunction) {
 
 TEST(ShadowNodeTest, handleLocalData) {
   auto eventDispatcher = std::shared_ptr<EventDispatcher const>();
-  auto componentDescriptor = TestComponentDescriptor(eventDispatcher);
+  auto componentDescriptor = TestComponentDescriptor(
+      ComponentDescriptorParameters{eventDispatcher, nullptr, nullptr});
   auto family = std::make_shared<ShadowNodeFamily>(
       ShadowNodeFamilyFragment{
           /* .tag = */ 9,
@@ -250,7 +255,8 @@ TEST(ShadowNodeTest, handleBacktracking) {
    */
 
   auto eventDispatcher = std::shared_ptr<EventDispatcher const>();
-  auto componentDescriptor = TestComponentDescriptor(eventDispatcher);
+  auto componentDescriptor = TestComponentDescriptor(
+      ComponentDescriptorParameters{eventDispatcher, nullptr, nullptr});
   auto props = std::make_shared<const TestProps>();
 
   auto familyAA = std::make_shared<ShadowNodeFamily>(
diff --git a/ReactCommon/fabric/uimanager/ComponentDescriptorProvider.h b/ReactCommon/fabric/uimanager/ComponentDescriptorProvider.h
@@ -14,17 +14,6 @@
 namespace facebook {
 namespace react {
 
-/*
- * Represents a collection of arguments that sufficient to construct a
- * `ComponentDescriptor`.
- */
-class ComponentDescriptorParameters {
- public:
-  EventDispatcher::Weak eventDispatcher;
-  ContextContainer::Shared contextContainer;
-  ComponentDescriptor::Flavor flavor;
-};
-
 /*
  * Callable signature that represents the signature of `ComponentDescriptor`
  * constructor. The callable returns a unique pointer conveniently represents an
@@ -61,10 +50,7 @@ ComponentDescriptor::Unique concreteComponentDescriptorConstructor(
       std::is_base_of<ComponentDescriptor, ComponentDescriptorT>::value,
       "ComponentDescriptorT must be a descendant of ComponentDescriptor");
 
-  return std::make_unique<ComponentDescriptorT const>(
-      parameters.eventDispatcher,
-      parameters.contextContainer,
-      parameters.flavor);
+  return std::make_unique<ComponentDescriptorT const>(parameters);
 }
 
 /*
diff --git a/ReactCommon/fabric/uimanager/Scheduler.cpp b/ReactCommon/fabric/uimanager/Scheduler.cpp
@@ -61,8 +61,8 @@ Scheduler::Scheduler(
   componentDescriptorRegistry_ = schedulerToolbox.componentRegistryFactory(
       eventDispatcher_, schedulerToolbox.contextContainer);
 
-  rootComponentDescriptor_ =
-      std::make_unique<const RootComponentDescriptor>(eventDispatcher_);
+  rootComponentDescriptor_ = std::make_unique<const RootComponentDescriptor>(
+      ComponentDescriptorParameters{eventDispatcher_, nullptr, nullptr});
 
   uiManager->setDelegate(this);
   uiManager->setComponentDescriptorRegistry(componentDescriptorRegistry_);
