Add setFSTagName option for mobile

[nfriedly]

This adds a new setFSTagName boolean option that sets the fsTagName attribute instead of componentAttribute and elementAttribute (e.g. MyCoolComponent instead of RCTView or Text instead of RCTTextView).

This allows us to generate better privacy selectors with less noise.

Ticket: https://fullstory.atlassian.net/browse/MOEN-374

We'll also want to coordinate with the KB team on updating the getting started guide after this change is released.

[nfriedly]

Ok, I've got this working. It has an automated test, and I've done some manual testing. It basically does what I set out to do. E.g. compare the selector for the "welcome to react native" text

Before https://app.staging.fullstory.com/ui/KWH/session/5388507044839424:4629571211558912 RCTRootContentView>RCTView[data-source-file="AppContainer.js"]>RCTView>RCTSafeAreaView[data-source-file="App.tsx"][data-element="SafeAreaView"][data-component="App"]>RCTScrollView>RCTCustomScrollView>RCTScrollContentView>RCTView[label="Welcome to\a React Native"][data-source-file="ImageBackground.js"][data-component="ImageBackground"]>RCTTextView[label="Welcome to\a React Native"][data-source-file="Header.js"][data-element="Text"]

After https://app.staging.fullstory.com/ui/KWH/session/5388507044839424:6109043999834112 RCTRootContentView>RCTView[data-source-file="AppContainer.js"]>RCTView>App[data-source-file="App.tsx"][data-element="SafeAreaView"]>ScrollView[data-source-file="ScrollView.js"][data-element="NativeDirectionalScrollView"]>RCTCustomScrollView>RCTScrollContentView>ImageBackground[label="Welcome to\a React Native"][data-source-file="ImageBackground.js"][data-element="View"]>RCTTextView[label="Welcome to\a React Native"][data-source-file="Header.js"][data-element="Text"]

It's definitely an improvement. But I feel like we could still do better.

For starters, we should use element as the tag name when we have an element but not a component. e.g.

- RCTTextView[label="Welcome to\a React Native"][data-source-file="Header.js"][data-element="Text"]
+ Text[label="Welcome to\a React Native"][data-source-file="Header.js"]

So, perhaps there should be a new optional attribute called tagName that would be

• component, if present
• element if there's no component but there is an element
• unset if there's neither a component nor an element

Secondly, I think it'd be nice to be able to turn specific attributes off by just setting them to false instead of a string name. Then, someone could use tagName instead of component/element and could turn off the source file if they wanted.

What do you think about that a) in general and b) as part of this PR?

[nfriedly]

One alternative idea: just make the component attribute take presidence if both are set to the same thing. Right now, if I do

      '@fullstory/annotate-react',
      {
        native: true,
        componentAttribute: 'fsTagName',
        elementAttribute: 'fsTagName',
      },

... then elementAttribute takes precedence:

- RCTSafeAreaView[data-source-file="App.tsx"][data-element="SafeAreaView"][data-component="App"]
+ SafeAreaView[data-source-file="App.tsx"]

If we just switched the order that component and element attributes are set, then it could be:

- RCTSafeAreaView[data-source-file="App.tsx"][data-element="SafeAreaView"][data-component="App"]
+ App[data-source-file="App.tsx"]

And it would still get the RCTTextView -> Text that I called out above.

[nfriedly]

I thought about it a little more and went ahead and did the a variation on the second option I proposed. This is the only time we can do that where it won't be a breaking change, and I think this is the right path to start with. I also updated the readme to suggest setting both to fsTagName for use with FullStory and to clarify the precedence rules.

Here's a new "after" session: https://app.staging.fullstory.com/ui/KWH/session/5388507044839424:5942778131841024

And a comparison of the selector from my original "before" and my new "after":

Before: RCTRootContentView>RCTView[data-source-file="AppContainer.js"]>RCTView>RCTSafeAreaView[data-source-file="App.tsx"][data-element="SafeAreaView"][data-component="App"]>RCTScrollView>RCTCustomScrollView>RCTScrollContentView>RCTView[label="Welcome to\a React Native"][data-source-file="ImageBackground.js"][data-component="ImageBackground"]>RCTTextView[label="Welcome to\a React Native"][data-source-file="Header.js"][data-element="Text"]

After: RCTRootContentView>View[data-source-file="AppContainer.js"]>RCTView>App[data-source-file="App.tsx"]>ScrollView[data-source-file="ScrollView.js"]>RCTCustomScrollView>RCTScrollContentView>ImageBackground[label="Welcome to\a React Native"][data-source-file="ImageBackground.js"]>Text[label="Welcome to\a React Native"][data-source-file="Header.js"]

It loses a tiny bit of information, but I think it generates a significantly better selector.

One thing I don't understand is how RCTView[data-source-file="AppContainer.js"]> turned into View[data-source-file="AppContainer.js"] (?)

Edit: Nevermind. I was looking at optimized selectors - the full selector has [data-element="View"] on the "before", but we just trim it out for the optimized one.

[nfriedly]

I'm not 100% about allowing the user to set custom names for the attributes though. It seems confusing to program a configuration that suggests setting componentAttribute and elementAttribute to a string fsTagName, which is meaningless from the user's perspective, no?

Is there an argument in favor of custom names vs an on-off setting? I think one reason in favor of the custom names is that it would be easier to retain backwards compatibility if the user is more flexible with the string configuration.

That's a fair point. My thinking here is that this is a fairly generic plugin, so making it configurable but not FullStory-specific seemed fit in better than adding FullStory-specific configuration to the plugin. Also, I think it reduces the "magic" a bit, which is a complaint we've gotten from some customers. (Although not, to my knowledge, any React Native customers...)

But, on the other hand, it does have "fullstory" right in the name, and I don't imagine there are a lot of people using this plugin who aren't FullStory customers. https://github.com/fullstorydev/fullstory-babel-plugin-annotate-react/network/dependents shows 42 repos, but at least 5 are from FullStory people. So, I'm not sure that it really matters.

On the balance, doing it this way just felt right to me.

But, If you feel strongly that a simple on-off toggle is better, we can change it.

[RyanCommits]

I'm not 100% about allowing the user to set custom names for the attributes though. It seems confusing to program a configuration that suggests setting componentAttribute and elementAttribute to a string fsTagName, which is meaningless from the user's perspective, no? Is there an argument in favor of custom names vs an on-off setting? I think one reason in favor of the custom names is that it would be easier to retain backwards compatibility if the user is more flexible with the string configuration.

That's a fair point. My thinking here is that this is a fairly generic plugin, so making it configurable but not FullStory-specific seemed fit in better than adding FullStory-specific configuration to the plugin. Also, I think it reduces the "magic" a bit, which is a complaint we've gotten from some customers. (Although not, to my knowledge, any React Native customers...)

But, on the other hand, it does have "fullstory" right in the name, and I don't imagine there are a lot of people using this plugin who aren't FullStory customers. https://github.com/fullstorydev/fullstory-babel-plugin-annotate-react/network/dependents shows 42 repos, but at least 5 are from FullStory people. So, I'm not sure that it really matters.

On the balance, doing it this way just felt right to me.

But, If you feel strongly that a simple on-off toggle is better, we can change it.

Coming from the point of the that this is a "generic plugin" that has functionality beyond FullStory, the custom string configurability does seem more appealing an option. However, the idea that this plugin is utilized for anything other than FullStory is a new thought to me.

From our readme:

This is a Babel plugin that annotates React components with stable attributes that can be used to search and select using FullStory.

I'm not sure we intend on supporting this plugin with any other motive than to improve it's interoperability with FullStory itself.

---

After doing some manual testing, I found a limitation to this "custom naming" feature. In our fullstory-react-native plugin, we only read an enumerated list of props that we pre-define. https://github.com/fullstorydev/fullstory-react-native/blob/master/ios/FullStory.mm#L193. You'll see that dataComponent and dataElement and others are hard-coded. Any other names for these props would not be seen.

Sorry that I missed this on my first review passthrough.

---

Another potential issue is if the user names componentAttribute and elementAttributeName, but not doesn't name it to fsTagName. Example:

  {
        native: true,
        componentAttribute: 'test',
        elementAttribute: 'test',
      },

The result would be that elementAttribute would just be left off without fsTagName containing any new information.

[nfriedly]

After doing some manual testing, I found a limitation to this "custom naming" feature. In our fullstory-react-native plugin, we only read an enumerated list of props that we pre-define. https://github.com/fullstorydev/fullstory-react-native/blob/master/ios/FullStory.mm#L193. You'll see that dataComponent and dataElement and others are hard-coded. Any other names for these props would not be seen.

Sorry that I missed this on my first review passthrough.

Another potential issue is if the user names componentAttribute and elementAttributeName, but not doesn't name it to fsTagName. Example:

 {
       native: true,
       componentAttribute: 'test',
       elementAttribute: 'test',
  },

The result would be that elementAttribute would just be left off without fsTagName containing any new information.

Yeah, this all stems from the generic vs fs-specific idea.

Maybe we should just make a single flag like setFSTagName. In that case, do you think we should set all the fields (fsTagName, dataComponent, and dataElement) or just fsTagName? I feel like the former is the behavior I'd expect from that config option, but the later is probably better. (And more akin to what the PR does in its current state.)

I suppose we could make boolean options for all of the attributes - if they're false, then don't set them. Then our recommended configuration could be something like this:

{
    plugins: [
      '@fullstory/react-native',
      ["@fullstory/annotate-react", {
        native: true,
        tagName: true,
        component: false,
        element: false,
        fileName: true,
      }]
    ]
}

Then customers who shipped the previous version could turn on component and element and use them to create backwards-compatible selectors.

[RyanCommits]

Yeah, this all stems from the generic vs fs-specific idea.

Maybe we should just make a single flag like setFSTagName. In that case, do you think we should set all the fields (fsTagName, dataComponent, and dataElement) or just fsTagName? I feel like the former is the behavior I'd expect from that config option, but the later is probably better. (And more akin to what the PR does in its current state.)

I suppose we could make boolean options for all of the attributes - if they're false, then don't set them. Then our recommended configuration could be something like this:

{
    plugins: [
      '@fullstory/react-native',
      ["@fullstory/annotate-react", {
        native: true,
        tagName: true,
        component: false,
        element: false,
        fileName: true,
      }]
    ]
}

Then customers who shipped the previous version could turn on component and element and use them to create backwards-compatible selectors.

I'm not sure I like adding that many config options. I'm also not sure on the value of adding the ability to turn each data point on and off.

Correct me if I'm wrong - My understanding is that the original motivation for this work is to adjust the following:

• the current selector RCTView[dataComponent="HomeScreen"][dataSourceFile="home-screen.jsx"]
• replace with new selector HomeScreen[dataSourceFile="home-screen.jsx"]

I was thinking that this required a single flag - i.e. setFSTagName, that would replace the fsTagName value with either dataElement or dataComponent and trim off either dataElement or dataComponent, based on whatever was used to replace fsTagName.

Let me know if I addressed your points with the correct understanding.

[nfriedly]

Ok, I've simplified it to a single setFSTagName boolean, I think that addresses the points you raised @RyanCommits - would you care to take another look?

Here's a sample session recorded with the updated version: https://app.staging.fullstory.com/ui/KWH/session/5388507044839424:4574346920525824 (selectors should be identical to the previous one.)

I also updated the docs links to point to https://developer.fullstory.com/mobile/react-native/auto-capture/set-tag-name/ rather than https://help.fullstory.com/hc/en-us/articles/360052419133-Getting-Started-with-FullStory-React-Native-Capture#01GRMBR5FK3R89ZQQ9F3V6Z5RB but I plan to update the former to include a link to the later.

[nfriedly]

Oh, drat. I should probably add a check to avoid overwriting the fsTagName if it's explicitly set by the customer.

[nfriedly]

Oh, drat. I should probably add a check to avoid overwriting the fsTagName if it's explicitly set by the customer.

It actually handles this correctly! I did some manual testing and added a unit test.

I think this is ready to go at this point.

[nfriedly]

Awesome! Would you like to get the update published to npm?

I'll work with the KB team on updating the getting started guide.

[RyanCommits]

Awesome! Would you like to get the update published to npm?

I'll work with the KB team on updating the getting started guide.

I don't have authorization to create a tag on this repo. Do you? I can ask either @patrick-fs or @JoshMiers-FS per permission, as they probably have admin rights.

[nfriedly]

Hum, you should probably be authorized to do that.