Skip to content

fix: TextInput npe in #29452 on react native side - #37302

Closed
jcdhlzq wants to merge 5 commits into
react:mainfrom
jcdhlzq:fix/textinput-npe
Closed

fix: TextInput npe in #29452 on react native side#37302
jcdhlzq wants to merge 5 commits into
react:mainfrom
jcdhlzq:fix/textinput-npe

Conversation

@jcdhlzq

@jcdhlzqjcdhlzq commented May 8, 2023

Copy link
Copy Markdown
Contributor

Summary:

Fix the TextInput npe in #29452 on react native side because if it is just avoided on App side by changing themes may cause side effects.

Changelog:

  • [ANDROID] [FIXED] - Fix TextInput NPE.

Test Plan:

Thanks to Fabriziobertoglio1987's wok, the problem in #29452 can be tested easily. This fixing works fine and causes no side effects.

The following video shows the test result. And the number 101 has been changed to 1001 in RNTester/TextInputKeyProp.js when testing.

fix-textinput-npe.mp4

@facebook-github-botfacebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label May 8, 2023
@analysis-bot

analysis-bot commented May 8, 2023•
edited
Loading

Copy link
Copy Markdown
PlatformEngineArchSize (bytes)Diff
androidhermesarm64-v8a8,732,070-519
androidhermesarmeabi-v7a8,043,412-510
androidhermesx869,221,423-512
androidhermesx86_649,074,399-513
androidjscarm64-v8a9,297,312-471
androidjscarmeabi-v7a8,486,080-469
androidjscx869,358,052-476
androidjscx86_649,614,639-470

Base commit: 2b932c3
Branch: main

@jcdhlzq
jcdhlzqforce-pushed the fix/textinput-npe branch from 0be7585 to bb21232CompareMay 8, 2023 12:59
@facebook-github-bot

facebook-github-bot commented May 9, 2023

Copy link
Copy Markdown
Contributor

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

cortinico
cortinico reviewed May 10, 2023
View reviewed changes
Comment threadpackages/react-native/ReactAndroid/build.gradle Outdated
jcdhlzq
jcdhlzq commented May 10, 2023
View reviewed changes
Comment threadpackages/react-native/ReactAndroid/src/main/res/views/textinput/values/styles.xml Outdated
@jcdhlzq

jcdhlzq commented May 11, 2023•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

After modifications above, I re-tested TextInputKeyProp.js with 101 changed to 1001 and TextInputExample.js. The npe problem doesn't occur and there no unexpected rendering effects, namely no side effects.

textinput-npe-fixed.mp4

@cipolleschi

cipolleschi commented May 11, 2023

Copy link
Copy Markdown
Contributor

cc. @cortinico@NickGerleman: do you think this is good now? If yes, I can proceed reimporting this and try to land it.

cortinico
cortinico approved these changes May 15, 2023
View reviewed changes

@cortinicocortinico left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@jcdhlzq

jcdhlzq commented May 15, 2023

Copy link
Copy Markdown
ContributorAuthor

@cipolleschi Is there anything else to do as the pr is approved and all checks have passed?

@cipolleschi

cipolleschi commented May 15, 2023

Copy link
Copy Markdown
Contributor

@jcdhlzq nope, I'll import it and try to land this.

@facebook-github-bot

facebook-github-bot commented May 15, 2023

Copy link
Copy Markdown
Contributor

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@jcdhlzq

jcdhlzq commented May 16, 2023

Copy link
Copy Markdown
ContributorAuthor

we got a warning from "Fackbook Internal - Linter", but i cannot see it in details.

@cipolleschi

cipolleschi commented May 16, 2023

Copy link
Copy Markdown
Contributor

we got a warning from "Fackbook Internal - Linter", but i cannot see it in details.

Yep, that's on us. Internally, we use BUCK to build React Native and TextInputShadowNode uses R from Java, but the dependency from the BUCK rule that knows about R is not stated in the TextInputShadowNode's BUCK files.

@cortinico and I are trying to figure out what is that dependency, fix it and landing. It could take a few days as I'm going to be at a Conference starting from later today.

I'm sorry for the delay.

@jcdhlzq

jcdhlzq commented May 16, 2023

Copy link
Copy Markdown
ContributorAuthor

we got a warning from "Fackbook Internal - Linter", but i cannot see it in details.

Yep, that's on us. Internally, we use BUCK to build React Native and TextInputShadowNode uses R from Java, but the dependency from the BUCK rule that knows about R is not stated in the TextInputShadowNode's BUCK files.

@cortinico and I are trying to figure out what is that dependency, fix it and landing. It could take a few days as I'm going to be at a Conference starting from later today.

I'm sorry for the delay.

I've re-sync the branch from upstream and now all checks have passed. @cipolleschi

@jcdhlzq

jcdhlzq commented May 16, 2023

Copy link
Copy Markdown
ContributorAuthor

we got a warning from "Fackbook Internal - Linter", but i cannot see it in details.

Yep, that's on us. Internally, we use BUCK to build React Native and TextInputShadowNode uses R from Java, but the dependency from the BUCK rule that knows about R is not stated in the TextInputShadowNode's BUCK files.
@cortinico and I are trying to figure out what is that dependency, fix it and landing. It could take a few days as I'm going to be at a Conference starting from later today.
I'm sorry for the delay.

I've re-sync the branch from upstream and now all checks have passed. @cipolleschi

It seems that the checks are diffrent from those after you importing the pr.

@cipolleschi

cipolleschi commented May 16, 2023

Copy link
Copy Markdown
Contributor

Yes, the internal linter runs only after importing the PR, so you updated the branch, but we need to reimport it to see if it passes.

@jcdhlzq

jcdhlzq commented May 22, 2023•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Shall we proceed with this PR this week? @cipolleschi

@cipolleschi

cipolleschi commented May 22, 2023

Copy link
Copy Markdown
Contributor

Hi @jcdhlzq! I'm back from the conference, I'll try to land this as soon as we can.

@facebook-github-botfacebook-github-bot added the Merged This PR has been merged. label May 23, 2023
@facebook-github-bot

facebook-github-bot commented May 23, 2023

Copy link
Copy Markdown
Contributor

@cipolleschi merged this pull request in 98789e9.

@fabOnReact

fabOnReact commented Jun 8, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor

A similar fix was already reviewed by Joshua Gross #27782 (comment) in 2020, so I did not further investigate at that time. Thanks for fixing the issue!

Relavant comment #17530 (comment)

Flewp pushed a commit to discord/react-native-archived that referenced this pull request May 16, 2024
@jcdhlzq@Flewp
[Upstreamed] fix: TextInput npe inreact#29452on react native side (r…
f1f15fe
…eact#37302)
Summary:
Fix the TextInput npe in react#29452 on react native side because if it is just avoided on App side by changing themes may cause side effects.
- [ANDROID] [FIXED] - Fix TextInput NPE.
<!-- Help reviewers and the release process by writing your own changelog entry.
Pick one each for the category and type tags:
[ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message
For more details, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
Pull Request resolved: react#37302
Test Plan:
Thanks to Fabriziobertoglio1987's wok, the problem in react#29452 can be tested easily. This fixing works fine and causes no side effects.
The following video shows the test result. And the number 101 has been changed to 1001 in RNTester/TextInputKeyProp.js when testing.
https://user-images.githubusercontent.com/23273745/236796702-e61a6fa9-9935-4179-9c5f-e9370d543657.mp4
Reviewed By: javache
Differential Revision: D45688987
Pulled By: cipolleschi
fbshipit-source-id: 4e13c19c10ed53cfcead79e66ab2e232369317e0
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cortinicocortinicocortinico approved these changes
+2 more reviewers
@NickGerlemanNickGerlemanNickGerleman left review comments
@fabOnReactfabOnReactfabOnReact left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

CLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.MergedThis PR has been merged.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@jcdhlzq@analysis-bot@facebook-github-bot@cipolleschi@fabOnReact@NickGerleman@cortinico