Skip to content

RFC: simplify Text and TextInput related queries #1184

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
mdjastrzebski opened this issue Oct 18, 2022 · 7 comments
Closed

RFC: simplify Text and TextInput related queries #1184

mdjastrzebski opened this issue Oct 18, 2022 · 7 comments
Assignees
Labels
discussion enhancement New feature or request

Comments

@mdjastrzebski
Copy link
Member

mdjastrzebski commented Oct 18, 2022

Describe the Feature

Currently when looking for Text and TextInput instances we compare to tree elements type with components exported from React Native. This is problematic because exported components are composite ones, while we try to work primarily with host components. It leads to methods like: isHostElementForType which try to navigate from host to composite parent of certain type to verify whether this is a host Text or TextInput.

There is an easy alternative, which is to compare ReactTestInstance.type field with string values of "Text" and "TextInput" which are currently used by these host components. The issue here is that the host types are not "standardised" and have used slightly different names in the past, e.g. RCTText instead of Text, so these could potentially change again.

In order to get the best of both worlds: robust code and being future proof I propose migrating to type matching against configurable host component names.

Possible Implementations

1. hostComponentNames config option

Extend config with hostComponentNames field. User will be able to pass custom values in case they use older version of RN (which we probably do not support) or if RN changes the naming in the future without need for release:

configure({
  hostComponentNames: {
    text: "Text",
    textInput: "TextInput",
  }
})

The default values are matching the current RN naming scheme.

2. Update queries and event handling

Modify queries and fireEvent code that looks for Text and TextInput to use hostComponentNames:

element.type === getConfig().hostComponentNames.text;
element.type === getConfig().hostComponentNames.textInput;

(Optional) 3. add hostComponentNames: "autodetect" option

Add autodetect option to configure that would run a simple auto-detection code:

configure({
  hostComponentNames: "autodetect";
})

Simple auto-detection code:

function autodetectHostComponentNames(): HostComponentNames {
  const { getByTestId } = render(
    <View>
      <Text testID="text">Hello</Text>
      <TextInput tystID="textInput" />
    </View>
  );

  return {
    text: getByTestId("text").props.testID,
    textInput: getByTestId("textInput").props.testID,
  }

This would allow users to quickly resolve any potential issues regarding host component name changes.

(Optional) 4. Suggest proper config setting for hostComponentNames option

Suggest proper values for hostComponentNames option when using auto-detect mode:

You are using host component name auto-detection mode. This might cause a slightly longer test execution time.
To make you tests run faster use the following code in your test setup file:

configure({
  hostComponentNames: {
    text: "New_Text",
    textInput: "New_TextInput",
  }
})

Where the actual names would be filled in by RNTL.

Pros and cons analysis

Benefits:

  • simpler logic for analysing component tree (no more composite parent lookups)
  • flexible in case of host component name change

Cons:

  • not 100% automatic

Related Issues

@thymikee
Copy link
Member

I wouldn't be opposed to use this kind of config. Additionally, can you post a RNTL perspective to this public API discussion? react-native-community/discussions-and-proposals#523 We could ask if displayName for host components to be defined as public facing, not an implementation detail. Or ask for better alternatives

@mdjastrzebski
Copy link
Member Author

Thanks @thymikee, I was looking especially for your feedback as you were proponent of current approach. We can contribute to the RN discussion, but I think we should proceed regardless of that, as any RN changes would take time to iron out and be merged into codebase.

@thymikee
Copy link
Member

Agree, it's only to secure the future of this approach

@AugustinLF
Copy link
Collaborator

autodetectHostComponentNames could eventually use some caching to make it a bit cheaper, so the rendering would be done only once by test. This ofc assumes that you can't use different versions of RN in the same test, but I guess they could switch to something manual in this specific case.

@pierrezimmermannbam
Copy link
Collaborator

This is a very interesting approach and I like the idea of suggesting proper config for users.

I'm thinking of an alternative approach where on failed query we'd match the component name in the config against the actual component name of RN (by running autoDetectHostComponentNames) and we may then suggest the appropriate component name to use in configure. This way failing tests would explain how to fix them so it's impossible to miss.

Also I think it would be possible that new releases of RNT directly support newer RN versions by changing the default config to match the component names of the RN version used by the user so that no additional configuration would be required

@mdjastrzebski
Copy link
Member Author

Running autoDetectHostComponentNames sounds like an interesting idea. That removes the need for feature 3.

We should take care though to suggest changing the config only if the found component name is different than the current config, as we want to avoid showing suggestion that would not fix anything.

@mdjastrzebski
Copy link
Member Author

Resolved by #1184

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants