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

Version 3.31.0 breaks InputGroup with redux form #4298

Closed
tgreen7 opened this issue Aug 29, 2020 · 13 comments · Fixed by #4323
Closed

Version 3.31.0 breaks InputGroup with redux form #4298

tgreen7 opened this issue Aug 29, 2020 · 13 comments · Fixed by #4323

Comments

@tgreen7
Copy link
Contributor

tgreen7 commented Aug 29, 2020

Environment

  • Package version(s): 3.31.0

Code Sandbox

working on 3.30.1
https://codesandbox.io/s/blueprint-sandbox-forked-n039t?file=/src/index.tsx

working

broken on 3.31.0
https://codesandbox.io/s/blueprint-sandbox-forked-327mz?file=/src/index.tsx

broken

Steps to reproduce

  1. Type in the input field in the sandbox
  2. Move cursor to beginning of the input and type again. Cursor jumps to the end of the input on version 3.31.0

Possible solution

Version 3.31.0 added the asyncControllableInput

@tnrich
Copy link
Contributor

tnrich commented Aug 31, 2020

Ya it seems like #4266 changed how async updates fired. This is definitely an issue for us

@adidahiya
Copy link
Contributor

@tnrich are you also using redux-form? curious if you are able to demonstrate a repro without using any external libraries

@tnrich
Copy link
Contributor

tnrich commented Sep 2, 2020

@adidahiya yes also using redux-form. I haven't tried to replicate outside of a redux-form context

@adidahiya
Copy link
Contributor

It looks like these issues: facebook/react#955 and facebook/react#14904. I'm going to try the workaround in facebook/react#14904 (comment):

What I've done to workaround is to make the uncontrolled (change value to defaultValue) with a onChange prop.

@tnrich
Copy link
Contributor

tnrich commented Sep 2, 2020

Thanks @adidahiya hopefully that works! Let us know when we can test out the new version.

@adidahiya
Copy link
Contributor

Update: using defaultValue all the time doesn't work, it is incompatible with a controlled API for InputGroup / NumericInput. Investigating other approaches.

@tnrich
Copy link
Contributor

tnrich commented Sep 14, 2020

Thanks @adidahiya let us know how it goes!

@adidahiya
Copy link
Contributor

I tried many different approaches and all were unsuccessful. You can see my progress at #4323. At this point I don't have a good path forward, and we are not hitting this issue with our internal applications. Maybe it's an anti-pattern to control inputs asynchronously like redux-form does? If you can come up with a solution which does not cause a regression for the issue described in #4262, I'm all ears.

@ejose19
Copy link
Contributor

ejose19 commented Sep 23, 2020

@adidahiya What about a prop to opt-out of the asyncControllableInput? As the change in #4266 was pretty trivial, a switch should be straightforward. Although there are better form libraries for react, a lot of people still use redux-form (which is not recommended on the usual cases as stated by the author)

@adidahiya adidahiya removed their assignment Oct 1, 2020
@adidahiya
Copy link
Contributor

I took another pass at a (slightly complicated) solution with fresh eyes, and I think I've got something working in #4323. I don't really want Blueprint users to have to think about toggling between async-capable inputs (which work around the IME composition bug) and regular inputs (which don't), because that choice would be hard to make across a software platform. The InputGroup building block should work for all locales out of the box.

@adidahiya
Copy link
Contributor

@tnrich @tgreen7 @ejose19 could you try out the latest release to verify the fix? It seems to work in the scenario from the initial code sandbox linked at the top of this thread, see my fork: https://codesandbox.io/s/blueprint-4298-fix-verification-vjz7n

@tnrich
Copy link
Contributor

tnrich commented Oct 19, 2020

@adidahiya I think it is working for us as far as we can tell. Can verify that the sandbox works as expected now. Thank you 🙏 🙏

@adidahiya
Copy link
Contributor

#4323 caused a regression for synchronous updates, so I have opened a PR to make async control optional. This will be a slight breaking change since you will need to enable the new prop manually in places where you expect async updates. PR is here: #4383

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants