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): make async control optional #4383

Merged
merged 3 commits into from
Oct 22, 2020

Conversation

adidahiya
Copy link
Contributor

@adidahiya adidahiya commented Oct 21, 2020

Fixes #4375

Checklist

  • Includes tests
  • Update documentation

Changes proposed in this pull request:

Using the suggestion from #4298 (comment), this change makes "async control" of InputGroup optional, after it was enabled across the board by default in #4266 (released in @blueprintjs/core@3.31.0).

I originally wanted to support all possible <input> use cases out-of-the-box by default with good UX for both synchronous value updates and async ones (see #4262, #4298). This turned out to be too much of a hassle, so I am now opting to make the complex async state handling behavior opt-in using a new prop called asyncControl.

asyncControl={true} should be used in places where applications expect that the state update to a controlled <InputGroup> will be asynchronous, either by explicit design in their state management logic, or through a library like redux-form. I think it makes sense to hide this complexity (and potential source of bugs) behind a flag; see the docs about this:

This is not broadly encouraged (a value returned from onChange should be sent back to the component as a controlled value synchronously)

Reviewers should focus on:

Fix the linked issue, and ensure no regression

@blueprint-bot
Copy link

[core] fix(InputGroup): make async control optional

Previews: documentation | landing | table

@blueprint-bot
Copy link

run new test only with React 16

Previews: documentation | landing | table

@blueprint-bot
Copy link

enable new prop in docs example

Previews: documentation | landing | table

Copy link
Contributor

@johnwiseheart johnwiseheart left a comment

Choose a reason for hiding this comment

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

Looks reasonable, nothing jumps out

@styu
Copy link
Contributor

styu commented Oct 21, 2020

technically this is a break for folks that were relying on the new async behavior, but now have to add the new prop to turn it on. what do we want to do about that?

@adidahiya
Copy link
Contributor Author

@styu yeah it is an API break for anyone who has upgraded to the range v3.31.0–3.34.0. I think that's ok since relying on async updates is kind of an edge case for this component, and I'd rather make the default / majority use case work properly.

@tgreen7 @tnrich @ejose19 heads up this API break is likely coming in the next minor release

@adidahiya
Copy link
Contributor Author

heads up @rynorris as well

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.

[InputGroup] Displayed value does not match value of 'value' prop
4 participants