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

[core] fix(InputGroup): async controlled updates #4266

Merged
merged 3 commits into from
Aug 11, 2020

Conversation

adidahiya
Copy link
Contributor

Alternative fix for issue described in #4262 and facebook/react#3926

Checklist

  • Includes tests
  • Update documentation

Changes proposed in this pull request:

Add a new private component, AsyncControllableInput, which works around the problems in the linked React issue, alleviating some bugs we've experienced with IME

Reviewers should focus on:

Test this in a real product and make sure it doesn't break anything

Screenshot

N/A

@blueprint-bot
Copy link

[core] fix(InputGroup): async controlled updates

Previews: documentation | landing | table

@rynorris
Copy link
Contributor

rynorris commented Aug 7, 2020

@adidahiya I've tested this PR against the product where we originally found the issue, and it does appear to fix it.

However, unless I'm mistaken, we're still relying on the state update here being synchronous. In practice, given how small and simple this component is, it seems likely this state update will always be synchronous (hence why it appears to fix the issue), but I don't think that's guaranteed by React at all.

I'm not sure if there's a better fix though, and maybe we're ok with it just being significantly less vulnerable to the issue than before?

@blueprint-bot
Copy link

fix: pass down inputRef to input

Previews: documentation | landing | table

@adidahiya
Copy link
Contributor Author

I tried fixing the tests but I don't see an obvious reason why they would fail only in React 15. I will come back to this later this week, but if you have any ideas I would welcome them in the meantime

@adidahiya
Copy link
Contributor Author

🤦 I had forgot to apply the react-lifecycles-compat polyfill to make getDerivedStateFromProps work in React 15...

@blueprint-bot
Copy link

apply react-lifecycles-compat polyfill

Previews: documentation | landing | table

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

Successfully merging this pull request may close these issues.

3 participants