From a45c809a32ea6ec9face83078c0fd8ac78ff814a Mon Sep 17 00:00:00 2001 From: Adi Dahiya Date: Wed, 21 Oct 2020 13:06:58 -0400 Subject: [PATCH 1/3] [core] fix(InputGroup): make async control optional --- .../forms/asyncControllableInput.tsx | 4 +- .../core/src/components/forms/inputGroup.tsx | 33 ++++++++----- .../core/src/components/forms/text-inputs.md | 9 ++-- .../core/test/controls/inputGroupTests.tsx | 35 +++++++++++++ .../forms/asyncControllableInputTests.tsx | 49 ++++++++++++++++++- 5 files changed, 112 insertions(+), 18 deletions(-) diff --git a/packages/core/src/components/forms/asyncControllableInput.tsx b/packages/core/src/components/forms/asyncControllableInput.tsx index 4081c433797..2015888b802 100644 --- a/packages/core/src/components/forms/asyncControllableInput.tsx +++ b/packages/core/src/components/forms/asyncControllableInput.tsx @@ -109,7 +109,7 @@ export class AsyncControllableInput extends React.PureComponent< } public render() { - const { isComposing, hasPendingUpdate: pendingUpdate, value, nextValue } = this.state; + const { isComposing, hasPendingUpdate, value, nextValue } = this.state; const { inputRef, ...restProps } = this.props; return ( +
{this.maybeRenderLeftElement()} - + {asyncControl ? ( + + ) : ( + + )} {this.maybeRenderRightElement()}
); diff --git a/packages/core/src/components/forms/text-inputs.md b/packages/core/src/components/forms/text-inputs.md index 05b0af6555c..0a49b34f9ee 100644 --- a/packages/core/src/components/forms/text-inputs.md +++ b/packages/core/src/components/forms/text-inputs.md @@ -30,10 +30,11 @@ all valid props for HTML `` elements and proxies them to that element in the DOM; the most common ones are detailed below. If controlled with the `value` prop, `InputGroup` has support for _asynchronous updates_, which may -occur with some form handling libraries like `redux-form`. This is not generally encouraged (a value -returned from `onChange` should generally be sent back to the component as a controlled `value` synchronously), -but there is basic support for it. Note that the input cursor may jump to the end of the input if the speed -of text entry (time between change events) is faster than the speed of the async update. +occur with some form handling libraries like `redux-form`. This is not broadly encouraged (a value +returned from `onChange` should be sent back to the component as a controlled `value` synchronously), +but there is basic support for it using the `asyncControl` prop. Note that the input cursor may jump +to the end of the input if the speed of text entry (time between change events) is faster than the +speed of the async update. @interface IInputGroupProps diff --git a/packages/core/test/controls/inputGroupTests.tsx b/packages/core/test/controls/inputGroupTests.tsx index 257e5ba5854..6f51db0c52d 100644 --- a/packages/core/test/controls/inputGroupTests.tsx +++ b/packages/core/test/controls/inputGroupTests.tsx @@ -67,4 +67,39 @@ describe("", () => { mount( (input = ref)} />); assert.instanceOf(input, HTMLInputElement); }); + + // this test was added to validate a regression introduced by AsyncControllableInput, + // see https://github.com/palantir/blueprint/issues/4375 + it("accepts controlled update truncating the input value", () => { + class TestComponent extends React.PureComponent< + { initialValue: string; transformInput: (value: string) => string }, + { value: string } + > { + public state = { value: this.props.initialValue }; + public render() { + return ; + } + private handleChange = (e: React.ChangeEvent) => { + this.setState({ + value: this.props.transformInput(e.target.value), + }); + }; + } + + const wrapper = mount( + value.substr(0, 3)} + />, + ); + + let input = wrapper.find("input"); + assert.strictEqual(input.prop("value"), "abc"); + + input.simulate("change", { target: { value: "abcd" } }); + input = wrapper.find("input"); + // value should not change because our change handler prevents it from being longer than characters + assert.strictEqual(input.prop("value"), "abc"); + }); }); diff --git a/packages/core/test/forms/asyncControllableInputTests.tsx b/packages/core/test/forms/asyncControllableInputTests.tsx index 6d619ef5edc..2e44a4760a6 100644 --- a/packages/core/test/forms/asyncControllableInputTests.tsx +++ b/packages/core/test/forms/asyncControllableInputTests.tsx @@ -14,6 +14,8 @@ * limitations under the License. */ +/* eslint-disable max-classes-per-file */ + import { assert } from "chai"; import { mount } from "enzyme"; import * as React from "react"; @@ -46,13 +48,58 @@ describe("", () => { assert.strictEqual(wrapper.childAt(0).type(), "input"); }); - it("accepts controlled updates", () => { + it("accepts controlled update 'hi' -> 'bye'", () => { const wrapper = mount(); assert.strictEqual(wrapper.find("input").prop("value"), "hi"); wrapper.setProps({ value: "bye" }); assert.strictEqual(wrapper.find("input").prop("value"), "bye"); }); + it("accepts async controlled update, optimistically rendering new value while waiting for update", done => { + class TestComponent extends React.Component<{ initialValue: string }, { value: string }> { + public state = { value: this.props.initialValue }; + public render() { + return ; + } + private handleChange = (e: React.ChangeEvent) => { + const newValue = e.target.value; + window.setTimeout(() => { + this.setState({ + value: newValue, + }); + }, 10); + }; + } + + const wrapper = mount(); + assert.strictEqual(wrapper.find("input").prop("value"), "hi"); + + wrapper.find("input").simulate("change", { target: { value: "hi " } }); + wrapper.update(); + + assert.strictEqual( + wrapper.find(AsyncControllableInput).prop("value"), + "hi", + "local state should still have initial value", + ); + // but rendered input should optimistically show new value + assert.strictEqual( + wrapper.find("input").prop("value"), + "hi ", + "rendered should optimistically show new value", + ); + + // after async delay, confirm the update + window.setTimeout(() => { + assert.strictEqual( + wrapper.find("input").prop("value"), + "hi ", + "rendered should still show new value", + ); + done(); + }, 20); + }); + it("triggers onChange events during composition", () => { const handleChangeSpy = spy(); const wrapper = mount(); From 521e8d676f3cf3ca2c80f5a8ef20cdb18d85c4ee Mon Sep 17 00:00:00 2001 From: Adi Dahiya Date: Wed, 21 Oct 2020 14:55:42 -0400 Subject: [PATCH 2/3] run new test only with React 16 --- .../forms/asyncControllableInputTests.tsx | 92 ++++++++++--------- packages/core/test/utils.ts | 4 + 2 files changed, 51 insertions(+), 45 deletions(-) diff --git a/packages/core/test/forms/asyncControllableInputTests.tsx b/packages/core/test/forms/asyncControllableInputTests.tsx index 2e44a4760a6..4fa90ecb13f 100644 --- a/packages/core/test/forms/asyncControllableInputTests.tsx +++ b/packages/core/test/forms/asyncControllableInputTests.tsx @@ -21,6 +21,8 @@ import { mount } from "enzyme"; import * as React from "react"; import { spy } from "sinon"; +import { sleep } from "../utils"; + // this component is not part of the public API, but we want to test its implementation in isolation import { AsyncControllableInput } from "../../src/components/forms/asyncControllableInput"; @@ -55,51 +57,6 @@ describe("", () => { assert.strictEqual(wrapper.find("input").prop("value"), "bye"); }); - it("accepts async controlled update, optimistically rendering new value while waiting for update", done => { - class TestComponent extends React.Component<{ initialValue: string }, { value: string }> { - public state = { value: this.props.initialValue }; - public render() { - return ; - } - private handleChange = (e: React.ChangeEvent) => { - const newValue = e.target.value; - window.setTimeout(() => { - this.setState({ - value: newValue, - }); - }, 10); - }; - } - - const wrapper = mount(); - assert.strictEqual(wrapper.find("input").prop("value"), "hi"); - - wrapper.find("input").simulate("change", { target: { value: "hi " } }); - wrapper.update(); - - assert.strictEqual( - wrapper.find(AsyncControllableInput).prop("value"), - "hi", - "local state should still have initial value", - ); - // but rendered input should optimistically show new value - assert.strictEqual( - wrapper.find("input").prop("value"), - "hi ", - "rendered should optimistically show new value", - ); - - // after async delay, confirm the update - window.setTimeout(() => { - assert.strictEqual( - wrapper.find("input").prop("value"), - "hi ", - "rendered should still show new value", - ); - done(); - }, 20); - }); - it("triggers onChange events during composition", () => { const handleChangeSpy = spy(); const wrapper = mount(); @@ -145,5 +102,50 @@ describe("", () => { assert.strictEqual(wrapper.find("input").prop("value"), "bye"); }); + + // this test only seems to work in React 16, where we don't rely on the react-lifecycles-compat polyfill + if (React.version.startsWith("16")) { + it("accepts async controlled update, optimistically rendering new value while waiting for update", async () => { + class TestComponent extends React.PureComponent<{ initialValue: string }, { value: string }> { + public state = { value: this.props.initialValue }; + public render() { + return ( + + ); + } + private handleChange = (e: React.ChangeEvent) => { + const newValue = e.target.value; + window.setTimeout(() => this.setState({ value: newValue }), 10); + }; + } + + const wrapper = mount(); + assert.strictEqual(wrapper.find("input").prop("value"), "hi"); + + wrapper.find("input").simulate("change", { target: { value: "hi " } }); + wrapper.update(); + + assert.strictEqual( + wrapper.find(AsyncControllableInput).prop("value"), + "hi", + "local state should still have initial value", + ); + // but rendered input should optimistically show new value + assert.strictEqual( + wrapper.find("input").prop("value"), + "hi ", + "rendered should optimistically show new value", + ); + + // after async delay, confirm the update + await sleep(20); + assert.strictEqual( + wrapper.find("input").prop("value"), + "hi ", + "rendered should still show new value", + ); + return; + }); + } }); }); diff --git a/packages/core/test/utils.ts b/packages/core/test/utils.ts index f3e47ef1864..b32a78fc7b8 100644 --- a/packages/core/test/utils.ts +++ b/packages/core/test/utils.ts @@ -32,3 +32,7 @@ export function findInPortal

(overlay: ReactWrapper

, selector: string) { } return portalChildren.find(selector); } + +export async function sleep(timeout?: number) { + return new Promise(resolve => window.setTimeout(resolve, timeout)); +} From 206bf49a4d5c33b67c11d923b8ac5d54fd3e1f45 Mon Sep 17 00:00:00 2001 From: Adi Dahiya Date: Wed, 21 Oct 2020 16:03:14 -0400 Subject: [PATCH 3/3] enable new prop in docs example --- .../docs-app/src/examples/core-examples/inputGroupExample.tsx | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/docs-app/src/examples/core-examples/inputGroupExample.tsx b/packages/docs-app/src/examples/core-examples/inputGroupExample.tsx index 1cc00ec2615..137590ed320 100644 --- a/packages/docs-app/src/examples/core-examples/inputGroupExample.tsx +++ b/packages/docs-app/src/examples/core-examples/inputGroupExample.tsx @@ -100,6 +100,7 @@ export class InputGroupExample extends React.PureComponent