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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 5 commits into from
Closed

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

wants to merge 5 commits into from

Conversation

jcdhlzq
Copy link
Contributor

@jcdhlzq jcdhlzq commented May 8, 2023

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-bot facebook-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
Copy link

analysis-bot commented May 8, 2023 •
edited
Loading

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,732,070 -519
android hermes armeabi-v7a 8,043,412 -510
android hermes x86 9,221,423 -512
android hermes x86_64 9,074,399 -513
android jsc arm64-v8a 9,297,312 -471
android jsc armeabi-v7a 8,486,080 -469
android jsc x86 9,358,052 -476
android jsc x86_64 9,614,639 -470

Base commit: 2b932c3
Branch: main

@jcdhlzq jcdhlzq force-pushed the fix/textinput-npe branch from 0be7585 to bb21232 Compare May 8, 2023 12:59
@facebook-github-bot
Copy link
Contributor

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

@jcdhlzq
Copy link
Contributor Author

jcdhlzq commented May 11, 2023 •
edited
Loading

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
Copy link
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
Copy link
Contributor

@cortinico cortinico left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@jcdhlzq
Copy link
Contributor Author

jcdhlzq commented May 15, 2023

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

@cipolleschi
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

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

@jcdhlzq
Copy link
Contributor Author

jcdhlzq commented May 16, 2023

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

@cipolleschi
Copy link
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
Copy link
Contributor Author

jcdhlzq commented May 16, 2023

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
Copy link
Contributor Author

jcdhlzq commented May 16, 2023

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
Copy link
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
Copy link
Contributor Author

jcdhlzq commented May 22, 2023 •
edited
Loading

Shall we proceed with this PR this week? @cipolleschi

@cipolleschi
Copy link
Contributor

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

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

@cipolleschi merged this pull request in 98789e9.

@fabOnReact
Copy link
Contributor

fabOnReact commented Jun 8, 2023 •
edited
Loading

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 that referenced this pull request May 16, 2024
@jcdhlzq @Flewp
[Upstreamed] fix: TextInput npe in facebook#29452 on react native side (
f1f15fe 
facebook#37302)

Summary:
Fix the TextInput npe in facebook#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: facebook#37302

Test Plan:
Thanks to Fabriziobertoglio1987's wok, the problem in facebook#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 free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabOnReact fabOnReact fabOnReact left review comments

@NickGerleman NickGerleman NickGerleman left review comments

@cortinico cortinico cortinico approved these changes

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This 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