Skip to content
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

feat: add more context to invalid text string parents #32544

Closed

Conversation

EvanBacon
Copy link
Contributor

Summary

The stack traces shown in LogBox often just point to fast refresh and don't provide much context into cases where a user adds a string outside of a Text element, this PR improves discoverability a little by adding the invalid text to the error message.

Changelog

[General] [Added] - Improved invalid text parent error message.

Test Plan

Tried it in a basic case: <View>foobar</View> will throw: Error: Text strings "foobar" must be rendered within a <Text> component.

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Expo Partner: Expo Partner Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Nov 5, 2021
@react-native-bot react-native-bot added the Type: Enhancement A new feature or enhancement of an existing feature. label Nov 5, 2021
@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: 88ebec9
Branch: main

@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 9,351,021 +52
android hermes armeabi-v7a 8,333,084 +50
android hermes x86 9,983,403 +46
android hermes x86_64 9,768,775 +45
android jsc arm64-v8a 10,665,552 +5
android jsc armeabi-v7a 9,311,090 +4
android jsc x86 10,777,609 +4
android jsc x86_64 11,216,919 +14

Base commit: 88ebec9
Branch: main

@ecreeth
Copy link
Contributor

ecreeth commented Nov 5, 2021

I believe that these changes must be applied in the react repo ReactNativeHostConfig & ReactFabricHostConfig

@yungsters
Copy link
Contributor

What @ecreeth said, but also… check out these two PRs by me:

tl;dr — I previously attempted a similar change, but was recommended to revert it because @acdlite and @sebmarkbage raised concerns with logging arbitrary strings which may be sensitive (e.g. passwords, banking information).

@Yonom
Copy link
Contributor

Yonom commented Nov 16, 2021

Perhaps a fair compromise is to log this information in debug builds only?

@yungsters
Copy link
Contributor

@Yonom — Some people unknowingly run debug builds in production (e.g. when you see RedBox displaying in some production apps), so there is still some risk there.

@EvanBacon — What do you think about only appending the string if an opt-in flag is set (e.g. global.debugInvalidTextStrings), and then changing the error message to inform people to do that to opt into the extra error text? (And maybe when the opt-in flag is set, remind them to remove it when they're done debugging.)

@yungsters
Copy link
Contributor

I am going to close this PR because the change has to be made in the React repository, as @ecreeth suggested above.

@yungsters yungsters closed this Nov 22, 2021
@EvanBacon EvanBacon deleted the @evanbacon/text/better-text-error branch December 15, 2021 00:05
@EvanBacon
Copy link
Contributor Author

Opened a PR for adding to react proper.

tl;dr — I previously attempted a similar change, but was recommended to revert it because @acdlite and @sebmarkbage raised concerns with logging arbitrary strings which may be sensitive (e.g. passwords, banking information).

I don't quite understand this issue since the error would be thrown when attempting to explicitly render a value to the screen, e.g. someone wrapped their password in JSX and passed it to the render method lol.

@yungsters
Copy link
Contributor

Yeah, that is the concern.

Either by design or by mistake, someone could conceivably render sensitive information (e.g. password, social security number, credit card information) as text without <Text>. Since errors are typically persisted to storage in plaintext, logging these strings is more likely to lead to unknowingly storing sensitive information in unsecured storage solutions.

@yungsters
Copy link
Contributor

For posterity, facebook/react#22725 seems to be the React PR. Let's move future discussion about this contribution to that repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Expo Partner: Expo Partner Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. Type: Enhancement A new feature or enhancement of an existing feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants