Skip to content

Commit

Permalink
Convert TextInput to Hooks
Browse files Browse the repository at this point in the history 
Summary:
Modernizing this code a bit more, converting it to hooks.

Changelog:
[General][Changed] Converted TextInput to use React hooks
[General][Fixed] TextInput now properly sends native the end selection location on change

(Note: this ignores all push blocking failures!)

Reviewed By: JoshuaGross

Differential Revision: D18581712

fbshipit-source-id: 62d6ea8489fa019ddf941c520930365f2c4887d8
  • Loading branch information
@TheSavior @facebook-github-bot
TheSavior authored and facebook-github-bot committed Nov 21, 2019
1 parent 8553e1a commit dff490d
Show file tree
Hide file tree
Showing 2 changed files with 279 additions and 246 deletions.
Loading

7 comments on commit dff490d

@kelset
Copy link
Contributor

@kelset kelset commented on dff490d Nov 27, 2019

Choose a reason for hiding this comment

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

😍

@garrettm
Copy link

Choose a reason for hiding this comment

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

@kelset @TheSavior this rewrite seems to cause issues with using controlled text/selection, the extra setState to remember lastNativeText/Selection seems to interact poorly with whatever logic lives on the native side. If you want to do anything besides just reflect back the text/selection values given to you by the text input itself, things get really wonky, and kinda unusable, pretty quick. I've had to fall back to using RN's TextInput completely uncontrolled and have a controlled wrapper around it that calls setNativeProps to adjust text/selection.

@kelset
Copy link
Contributor

@kelset kelset commented on dff490d May 21, 2020

Choose a reason for hiding this comment

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

@garrettm can you please open a dedicated issue with a repro on latest (0.62)? This is not the best place to report 😅

@mikefogg
Copy link

Choose a reason for hiding this comment

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

@garrettm Since this is actually now broken, do you have an example of how you fixed it temporarily using setNativeProps? I'd love to not have to downgrade to get this working again :) (Waiting on some more movement in #29063)

Thanks!

@garrettm
Copy link

@garrettm garrettm commented on dff490d Jul 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@mikefogg our fix is pretty specific to our codebase, the high level is:

  • never send text/selection via props in render
  • create logic to try to track what you expect the native side to be showing, and if they differ, in render/update (depending on plaintext/attributed) send the text/selection values you want
  • on the JS side coalesce selection + text changes, they need to happen together to have proper logic for changing the string/selection in custom ways
  • when setting selection on Android via setNativeProps, have to re-set it back to null after a 1ms delay, or it'll get stuck
  • various other minor hacks (for example, attributed text needs different logic than plaintext)

It's pretty involved, has a bunch of hacks, and doesn't work all the time. If I was gonna do it again, I'd probably just fork the native code...

@kelset
Copy link
Contributor

@kelset kelset commented on dff490d Jul 15, 2020

Choose a reason for hiding this comment

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

@mikefogg @garrettm can you please move your comments in the issue Mike linked? That way your insights can benefit everyone more easily. I've linked the issue in the contributors discord so that hopefully it will be surfaced more quickly to the FB team

@garrettm
Copy link

Choose a reason for hiding this comment

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

@kelset copied my comment over there 👍

Please sign in to comment.