Skip to content

Commit 592e6c5

Browse files
sherginfacebook-github-bot
authored andcommitted
Fabric: Restoring the original purpose of StateData class
Summary: This diff restores the original role of StateData as a dummy class that has only one purpose - designate that there is no state associated with the node. I believe having an abstract StateData class for all data types is not necessary and actually slightly harmful. And here the reasons: * Having virtual dispatch enabled for classes introduces some overhead, and it should be used only if it is absolutely necessary. In this case, we don't need to have virtual dispatch enabled for Data classes because it's already enabled for State classes; therefore all virtual resolution is done there and that's sufficient. No need to pay for what we don't need. * Continuing the previous point, yes, we expect some API being exposed for Data classes (such as `getDynamic`), but introducing a base abstract class for that is *not* an idiomatic C++ solution; in C++ it's by design that most of the template contracts are actually duck-typed. This is a well-known design issue/concern that will be addressed with an effort called "Concepts" in C++20. Anyway, we should not use abstract classes as *only* indication of conformance to some interface. * StateData does not introduce any convenience. As it is clear from several subclass implementations, we don't really use base methods from it. * The previous issue actually happens especially because of the interface of StateData class. Consider constructor that accepts `folly::dynamic` and does nothing, it actually useless because it's impossible to use it unless it's called inside superclass's constructor. That's the case because C++ does not support virtual dispatch for constructors (for good). * Avoiding subclassing StateData counter-intuitively enforces the desired conformance to imaginary interface: it does not compile otherwise. Reviewed By: JoshuaGross Differential Revision: D15342338 fbshipit-source-id: 1f04f7fc5247499e7cfc09cd4b3cccffdc0b6b06
1 parent 9c3f4c0 commit 592e6c5

File tree

4 files changed

+13
-53
lines changed

4 files changed

+13
-53
lines changed

ReactCommon/fabric/components/modal/ModalHostViewState.cpp

+1-6
Original file line numberDiff line numberDiff line change
@@ -6,17 +6,12 @@
66
*/
77

88
#include "ModalHostViewState.h"
9-
#include <glog/logging.h>
10-
11-
#ifdef ANDROID
12-
#include <folly/dynamic.h>
13-
#endif
149

1510
namespace facebook {
1611
namespace react {
1712

1813
#ifdef ANDROID
19-
const folly::dynamic ModalHostViewState::getDynamic() const {
14+
folly::dynamic ModalHostViewState::getDynamic() const {
2015
return folly::dynamic::object("screenWidth", screenSize.width)(
2116
"screenHeight", screenSize.height);
2217
}

ReactCommon/fabric/components/modal/ModalHostViewState.h

+6-4
Original file line numberDiff line numberDiff line change
@@ -7,24 +7,26 @@
77

88
#pragma once
99

10-
#include <react/core/StateData.h>
1110
#include <react/graphics/Float.h>
1211
#include <react/graphics/Geometry.h>
1312
#include <react/graphics/conversions.h>
1413

14+
#ifdef ANDROID
15+
#include <folly/dynamic.h>
16+
#endif
17+
1518
namespace facebook {
1619
namespace react {
1720

1821
/*
1922
* State for <BottomSheetView> component.
2023
*/
21-
class ModalHostViewState : public StateData {
24+
class ModalHostViewState final {
2225
public:
2326
using Shared = std::shared_ptr<const ModalHostViewState>;
2427

2528
ModalHostViewState(){};
2629
ModalHostViewState(Size screenSize_) : screenSize(screenSize_){};
27-
virtual ~ModalHostViewState() = default;
2830

2931
#ifdef ANDROID
3032
ModalHostViewState(folly::dynamic data)
@@ -35,7 +37,7 @@ class ModalHostViewState : public StateData {
3537
const Size screenSize{};
3638

3739
#ifdef ANDROID
38-
virtual const folly::dynamic getDynamic() const override;
40+
folly::dynamic getDynamic() const;
3941
#endif
4042

4143
#pragma mark - Getters

ReactCommon/fabric/core/state/StateData.cpp

-30
This file was deleted.

ReactCommon/fabric/core/state/StateData.h

+6-13
Original file line numberDiff line numberDiff line change
@@ -17,23 +17,16 @@ namespace facebook {
1717
namespace react {
1818

1919
/*
20-
* Base class for state data.
21-
* Must be used to provide getDynamic for Android.
20+
* Dummy type that is used as a placeholder for state data for nodes that
21+
* don't have a state.
2222
*/
23-
class StateData {
24-
public:
23+
struct StateData final {
2524
using Shared = std::shared_ptr<void>;
2625

27-
StateData() {}
28-
2926
#ifdef ANDROID
30-
StateData(folly::dynamic data) {}
31-
32-
// Destructor must either be virtual or protected if we have any
33-
// virtual methods
34-
virtual ~StateData();
35-
36-
virtual const folly::dynamic getDynamic() const;
27+
StateData() = default;
28+
StateData(folly::dynamic data){};
29+
folly::dynamic getDynamic() const;
3730
#endif
3831
};
3932

0 commit comments

Comments
 (0)