-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
[HOLD for payment 2023-05-05] [HOLD for payment 2023-04-20] [$1000] Text strings must be rendered within a <Text> component #16683
Comments
Triggered auto assignment to @miljakljajic ( |
Bug0 Triage Checklist (Main S/O)
|
Job added to Upwork: https://www.upwork.com/jobs/~01ef3651f57da6d5c9 |
Current assignee @miljakljajic is eligible for the External assigner, not assigning anyone new. |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @mananjadhav ( |
Current assignee @luacmartins is eligible for the External assigner, not assigning anyone new. |
@luacmartins Is it possible to share the Crashlytics logs? |
Stack trace:
Logs:
|
Looks like something related to As a reminder, please make sure that all proposals are not workarounds and that any and all attempt to fix the issue holistically have been made before proceeding with a solution. Proposals to change our Feel free to drop a note in #expensify-open-source with any questions. |
I believe this crash came from attachment modal which was a recent regression and now fixed. |
So it seems we should look for all string && { code } blocks and fix them
|
@luacmartins if these errors are new and should've appeared after recent changes in staging here are potential problems: App/src/components/Picker/Picker.js Lines 179 to 196 in 8a15103
and App/src/components/Picker/Picker.js Lines 258 to 263 in 8a15103
we shouldn't use these are coming from this PR #16449 |
@situchan @luacmartins could be because of comments in return function |
@alexxxwork nope, this happened on iOS as well. |
It seems like the exceptions haven't happened on v1.2.92-0, which means it could be related to this deploy blocker. That being said, I do think that we should update all instances of the Please post a full proposal that addresses all instances of this pattern in App. |
ProposalPlease re-state the problem that we are trying to solve in this issue.Text strings must be rendered within a component. What is the root cause of that problem?The possible root causes of the issue are as follows: -
What changes do you think we should make in order to solve the problem?The possible solutions to the issue are as follows: -
What alternative solutions did you explore? (Optional)NA |
@luacmartins I believe these are all cases now ProposalPlease re-state the problem that we are trying to solve in this issue.We get What is the root cause of that problem?We need to search for pattern string && JSX to eliminate such cases What changes do you think we should make in order to solve the problem?we need to fix this (replace with Boolean(*), add to PropTypes): List of files to fixApp/src/components/Picker/Picker.js Lines 179 to 196 in 8a15103
App/src/components/Picker/Picker.js Lines 258 to 263 in 8a15103
App/src/components/AttachmentModal.js Lines 271 to 305 in 2398744
App/src/components/MenuItem.js Line 93 in 2398744
App/src/components/MenuItem.js Line 159 in 2398744
App/src/components/OptionRow.js Line 274 in 2398744
App/src/components/RadioButtonWithLabel.js Lines 70 to 75 in 2398744
Lines 49 to 59 in 2398744
App/src/components/ImageView/index.native.js Line 178 in 2398744
App/src/components/ScreenWrapper/index.js Line 123 in 2398744
App/src/components/TextInput/BaseTextInput.js Line 247 in 2398744
App/src/components/TextInput/BaseTextInput.js Line 319 in 2398744
App/src/components/TextInput/BaseTextInput.js Line 331 in 2398744
App/src/components/ValidateCode/ExpiredValidateCodeModal.js Lines 95 to 103 in 2398744
Line 152 in 2398744
App/src/pages/ReportSettingsPage.js Line 219 in 2398744
App/src/pages/ReportSettingsPage.js Line 229 in 2398744
App/src/pages/EnablePayments/TermsPage/LongTermsForm.js Lines 68 to 82 in 2398744
App/src/pages/home/ReportScreen.js Line 267 in 2398744
App/src/pages/settings/InitialSettingsPage.js Line 303 in 2398744
App/src/pages/signin/PasswordForm.js Line 218 in 2398744
App/src/pages/signin/SignInPage.js Line 119 in 2398744
App/src/stories/Composer.stories.js Line 67 in 2398744
|
@alexxxwork your proposal looks good. |
📣 @alexxxwork You have been assigned to this job by @luacmartins! |
@luacmartins @aldo-expensify @miljakljajic I've posted a comment on all the PRs so that the contributors are aware about this in the future. |
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.7-3 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue: If no regressions arise, payment will be issued on 2023-05-05. 🎊 After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.
As a reminder, here are the bonuses/penalties that should be applied for any External issue:
|
BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:
|
I think we're good to close this one out? @miljakljajic the second checklist is not needed. We merged a PR to update the checklist. |
Makes sense, closing! |
Problem
Coming from Firebase Crashlytics, there are several non-fatal exceptions for
Text strings must be rendered within a <Text> component.
in v1.2.91. We should fix these.Why is this important
Guarantees App stability
Solution
Find a way to reproduce the exception and prevent text nodes from being rendered outside of a
<Text>
component. I suspect that the exception is coming from this line, although I couldn't reproduce the issue.Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: