iOS Fabric implementation #79

RyanCommits · 2023-10-31T14:14:49Z

This work seeks to solve an issue where props are statically defined in Fabric and cannot set our FullStory props onto views.

jwise

I see one 'in review' on DOC-1863, one 'unstarted', and one 'design approved'. I still have some open questions from a few weeks ago, and I think @jth0024 does as well, that need to be answered before we merge this approach (probably we should add Jordan as a reviewer on DOC-1863, also). I think it is possible that we can make this design work, but I'm not sure we're ready to commit to it based on the state of the design doc?

So I guess -1 for now on that front (and certainly there are style / functionality changes that are currently needed), but I don't fundamentally object to this design if we can show that it works well.

jwise · 2023-11-01T01:52:50Z

ios/FullStory.mm

@@ -216,16 +216,61 @@ - (void)log:(double)logLevel message:(NSString *)message {

 static const char *_rctview_previous_attributes_key = "associated_object_rctview_previous_attributes_key";

+static void set_fsClass(id json, RCTView *view) {


Would it be more idiomatic to have this as an extension on RCTView (i.e., -[RCTView _fs_set_fsClass:])?

I think a static function is the correct idiom to use when the functionality's use is limited to the current file. Extensions are meant to allow use across files - they add functions to the lookup table, and carry the risk of being referenced outside of their intended use.

Ok, fair enough. (Generally I tend to prefix static methods with _, and I tend to prefer the parameter order object_verb_noun(object, parameters...), which in this case would translate to _rctview_set_fsClass(RCTView *view, id json), just to scope things a little more tightly -- like we do two lines prior, scoping _rctview_previous_attributes_key by name -- but that could be a matter of taste.)

ios/FullStory.mm

jwise · 2023-11-01T02:02:00Z

src/index.ts

+interface NativeCommands {
+  fsClass: (viewRef: React.ElementRef<FSComponentType>, fsClass: string) => void;
+  fsAttribute: (viewRef: React.ElementRef<FSComponentType>, fsAttribute: object) => void;
+  fsTagName: (viewRef: React.ElementRef<FSComponentType>, fsTagName: string) => void;
+  dataElement: (viewRef: React.ElementRef<FSComponentType>, dataElement: string) => void;
+  dataSourceFile: (viewRef: React.ElementRef<FSComponentType>, dataElement: string) => void;
+  dataComponent: (viewRef: React.ElementRef<FSComponentType>, dataElement: string) => void;
+}
+
+const Commands: NativeCommands = codegenNativeCommands<NativeCommands>({
+  supportedCommands: [
+    'fsClass',
+    'fsAttribute',
+    'fsTagName',
+    'dataElement',
+    'dataComponent',
+    'dataSourceFile',
+  ],
+});


As I mentioned in the design doc, I'm still a little nervous about doing these sequentially. Do we know what thread JavaScript runs on? I guess it might be okay iff a view is added to the hierarchy only after our applyFSPropertiesWithRef completes. But if that's the case we need to document that, ideally, in a way that shows why that is the case.

I believe your concerns were addressed in the discussion and design doc. I don't think it's appropriate to write an essay about React Native's architecture here.

Yes, my concerns about threading of this were addressed in the design doc. It might be worth adding a comment to, if nothing else, point to the discussion in the design doc for why this is ok.

comment added

jwise · 2023-11-01T02:03:49Z

src/index.ts

+      // @ts-expect-error `currentProps` is missing in `NativeMethods`
+      const fsClass = element.currentProps['fsClass'];


I wonder if we could DRY this currentProps is missing thing by pulling currentProps into a local. And broadly I wonder if we could DRY it by just iterating all of the supportedCommands or something rather than naming each one three times...

I looked at this and as far as I can tell, it's not completely trivial. Interface properties are not iterable and only exist at compile time unless customers implement custom transformers. Plugin support for custom transformers continues to be an open issue after five years. This aesthetic issue should not be a blocking concern for this PR.

Wow, big oof that we can't iterate over an interface.

I was kind of thinking something like (pseudocode):

for (prop in ['fsClass', 'fsAttribute', 'fsTagName', ...]) { // @ts-expect-error ... const val = element.currentProps[prop]; if (val) { Commands[prop](element, val); /* will we need a ts waiver here? */ } }

Something like this definitely has the chance to grow copy-and-paste errors if it needs to be touched later so if we can avoid repeating something six times, that might be nice. If we were feeling extremely fancy, that list (['fsClass', ...]) could also be a const elsewhere that we could pass as the parameter to codegenNativeCommands, for one more repetition eliminated!

Or, if nothing else, hoisting out const props = element.currentProps;, so we only have to @ts-expect-error once. What do you think about these?

Hoisted the element.currentProps so we only have the ts-expect-error once.

Improved types so copy-and-paste errors will be unlikely since it has to match the NativeCommand type.

RyanCommits · 2023-11-01T14:39:43Z

Sorry @jwise , I should've labeled this PR as work in progress. I wanted to get my JS changes up so that @martin-fs can fill in his iOS work after. Martin has the latest Obj-c code which isn't in this PR yet

prashanthsadasivan-fullstory

the swizzles look good to me. some various small comments, but LGTM from my perspective.

prashanthsadasivan-fullstory · 2023-11-13T20:49:51Z

ios/FullStory.mm

+    // we separately swizzle each class manually
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wundeclared-selector"
+    SWIZZLE_HANDLE_COMMAND(RCTViewComponentView);


i like this, super easy to understand what the swizzles are.

prashanthsadasivan-fullstory · 2023-11-13T20:55:11Z

ios/FullStory.mm

+                if (existingMethod) {
+                    IMP existingImplementation = method_getImplementation(existingMethod);
+                    if (existingImplementation != swizzledViewComponentViewCommandImplementation) {
+                        NSAssert(strncmp(className, "RCT", 3) != 0, @"React Native framework class %s needs handleCommand support! Please contact FullStory support with this message.", className);


i'm not the most familiar with NSAsserts really - does this quit the app like a crash? I guess like, if a developer ends up in this block, would they be able to still use their app in debug mode? it would kinda suck if a customer has to update their RN fullstory version in the middle of a development session because we're NSAsserting something here.

Yes, it would crash the app when it is running in debug mode. I agree that this would be very disruptive to the developer. However, it's kind of unsafe for them to use the SDK if this is the case. If we are worried that this assert will be triggered, we could instead swizzle all subclasses per the original design. Or, we could add a terminate-with-error-message method to the SDK in a followup PR and terminate with this error message instead of NSAssert.

I'm ok with this being a crashing NSAssert -- especially because a developer wanting to work around it doesn't need to wait for us to release a new plugin version, but instead can make the modification themselves in their local tree. Nice work on checking this in debug.

ios/FullStory.mm

jwise

ObjC changes LGTM. I suggested a few comments and a few style changes, and it would be nice to see something to shore up the DRY situation down there in TS land, but I trust you to make those changes as appropriate. Looks like an approach that is going to work out!

Co-authored-by: Joshua Wise <joshua@joshuawise.com>

RyanCommits assigned JoshMiers-FS, martin-fs and jwise Oct 31, 2023

jwise suggested changes Nov 1, 2023

View reviewed changes

RyanCommits changed the title ~~iOS Fabric implementation~~ [WIP] - iOS Fabric implementation Nov 1, 2023

martin-fs marked this pull request as draft November 1, 2023 16:35

martin-fs and others added 6 commits November 8, 2023 20:39

Initial prototype

869454a

ios-fabric

d2047f4

update types

9312d02

remove comments

dffcceb

fix npm vulnerabilities

f01f58b

Swizzle handleCommand for all RCT*ComponentView classes

e541035

martin-fs force-pushed the ryanwang/ios-fabric branch from 2301cc4 to e541035 Compare November 9, 2023 05:22

martin-fs changed the title ~~[WIP] - iOS Fabric implementation~~ iOS Fabric implementation Nov 9, 2023

martin-fs marked this pull request as ready for review November 9, 2023 19:33

martin-fs assigned prashanthsadasivan-fullstory Nov 9, 2023

prashanthsadasivan-fullstory approved these changes Nov 13, 2023

View reviewed changes

run only on iOS and fix ref passing

cc0594d

jwise reviewed Nov 18, 2023

View reviewed changes

ios/FullStory.mm Outdated Show resolved Hide resolved

jwise reviewed Nov 18, 2023

View reviewed changes

ios/FullStory.mm Show resolved Hide resolved

jwise reviewed Nov 18, 2023

View reviewed changes

ios/FullStory.mm Outdated Show resolved Hide resolved

jwise approved these changes Nov 18, 2023

View reviewed changes

rwang-fs and others added 5 commits November 20, 2023 16:46

improve types

6e80826

Update ios/FullStory.mm

8d9634a

Co-authored-by: Joshua Wise <joshua@joshuawise.com>

Update ios/FullStory.mm

9179597

Co-authored-by: Joshua Wise <joshua@joshuawise.com>

Update ios/FullStory.mm

49a389f

Co-authored-by: Joshua Wise <joshua@joshuawise.com>

add threading comment

b3edbe2

RyanCommits merged commit 69c2b45 into master Nov 20, 2023

RyanCommits deleted the ryanwang/ios-fabric branch November 20, 2023 22:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

iOS Fabric implementation #79

iOS Fabric implementation #79

RyanCommits commented Oct 31, 2023

jwise left a comment

jwise Nov 1, 2023

martin-fs Nov 9, 2023

jwise Nov 18, 2023

jwise Nov 1, 2023

martin-fs Nov 9, 2023

jwise Nov 18, 2023

RyanCommits Nov 20, 2023

jwise Nov 1, 2023

martin-fs Nov 9, 2023

jwise Nov 18, 2023

RyanCommits Nov 20, 2023

RyanCommits commented Nov 1, 2023

prashanthsadasivan-fullstory left a comment

prashanthsadasivan-fullstory Nov 13, 2023

prashanthsadasivan-fullstory Nov 13, 2023

martin-fs Nov 14, 2023

jwise Nov 18, 2023

jwise left a comment

		@@ -216,16 +216,61 @@ - (void)log:(double)logLevel message:(NSString *)message {

		static const char *_rctview_previous_attributes_key = "associated_object_rctview_previous_attributes_key";

		static void set_fsClass(id json, RCTView *view) {

		// @ts-expect-error `currentProps` is missing in `NativeMethods`
		const fsClass = element.currentProps['fsClass'];

iOS Fabric implementation #79

iOS Fabric implementation #79

Conversation

RyanCommits commented Oct 31, 2023

jwise left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RyanCommits commented Nov 1, 2023

prashanthsadasivan-fullstory left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwise left a comment

Choose a reason for hiding this comment