From 13a3788c541dfcaf0dd8e39ad35271742838ca7a Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 13 Mar 2019 16:20:13 +0000 Subject: [PATCH 01/12] Improve async useEffect warning (#15104) --- .../src/ReactFiberCommitWork.js | 21 ++++++++++--------- .../src/__tests__/ReactHooks-test.internal.js | 6 +++--- 2 files changed, 14 insertions(+), 13 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.js b/packages/react-reconciler/src/ReactFiberCommitWork.js index cd402d0e3ac43..35ed489c6a26a 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.js @@ -346,22 +346,23 @@ function commitHookEffectList( } else if (typeof destroy.then === 'function') { addendum = '\n\nIt looks like you wrote useEffect(async () => ...) or returned a Promise. ' + - 'Instead, you may write an async function separately ' + - 'and then call it from inside the effect:\n\n' + - 'async function fetchComment(commentId) {\n' + - ' // You can await here\n' + - '}\n\n' + + 'Instead, write the async function inside your effect ' + + 'and call it immediately:\n\n' + 'useEffect(() => {\n' + - ' fetchComment(commentId);\n' + - '}, [commentId]);\n\n' + - 'In the future, React will provide a more idiomatic solution for data fetching ' + - "that doesn't involve writing effects manually."; + ' async function fetchData() {\n' + + ' // You can await here\n' + + ' const response = await MyAPI.getData(someId);\n' + + ' // ...\n' + + ' }\n' + + ' fetchData();\n' + + '}, [someId]);\n\n' + + 'Learn more about data fetching with Hooks: https://fb.me/react-hooks-data-fetching'; } else { addendum = ' You returned: ' + destroy; } warningWithoutStack( false, - 'An Effect function must not return anything besides a function, ' + + 'An effect function must not return anything besides a function, ' + 'which is used for clean-up.%s%s', addendum, getStackByFiberInDevAndProd(finishedWork), diff --git a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js index 202158a565d75..a9313f2fd5477 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js @@ -645,20 +645,20 @@ describe('ReactHooks', () => { const root1 = ReactTestRenderer.create(null); expect(() => root1.update()).toWarnDev([ - 'Warning: An Effect function must not return anything besides a ' + + 'Warning: An effect function must not return anything besides a ' + 'function, which is used for clean-up. You returned: 17', ]); const root2 = ReactTestRenderer.create(null); expect(() => root2.update()).toWarnDev([ - 'Warning: An Effect function must not return anything besides a ' + + 'Warning: An effect function must not return anything besides a ' + 'function, which is used for clean-up. You returned null. If your ' + 'effect does not require clean up, return undefined (or nothing).', ]); const root3 = ReactTestRenderer.create(null); expect(() => root3.update()).toWarnDev([ - 'Warning: An Effect function must not return anything besides a ' + + 'Warning: An effect function must not return anything besides a ' + 'function, which is used for clean-up.\n\n' + 'It looks like you wrote useEffect(async () => ...) or returned a Promise.', ]); From d822d4bbe7fefee12dfd87cd799119714ffa5bac Mon Sep 17 00:00:00 2001 From: Mateusz Date: Thu, 14 Mar 2019 22:40:09 +0100 Subject: [PATCH 02/12] Don't set the first option as selected in select tag with `size` attribute (#14242) * Set 'size' attribute to select tag if it occurs before appending options * Add comment about why size is assigned on select create. Tests I added some more clarification for why size must be set on select element creation: - In the source code - In the DOM test fixture - In a unit test * Use let, not const in select tag stub assignment --- .../src/components/fixtures/selects/index.js | 28 +++++++++++++++++++ .../src/__tests__/ReactDOMSelect-test.js | 26 +++++++++++++++++ .../react-dom/src/client/ReactDOMComponent.js | 23 +++++++++++---- 3 files changed, 71 insertions(+), 6 deletions(-) diff --git a/fixtures/dom/src/components/fixtures/selects/index.js b/fixtures/dom/src/components/fixtures/selects/index.js index e83bc3efd6676..c0423b6df853e 100644 --- a/fixtures/dom/src/components/fixtures/selects/index.js +++ b/fixtures/dom/src/components/fixtures/selects/index.js @@ -202,6 +202,34 @@ class SelectFixture extends React.Component { + + + + No options should be selected. + + +
+ +
+ +

+ Notes: This happens if size is assigned after + options are selected. The select element picks the first item by + default, then it is expanded to show more options when{' '} + size is assigned, preserving the default selection. +

+

+ This was introduced in React 16.0.0 when options were added before + select attribute assignment. +

+ ); } diff --git a/packages/react-dom/src/__tests__/ReactDOMSelect-test.js b/packages/react-dom/src/__tests__/ReactDOMSelect-test.js index bd87c4543e95f..cc7285d155ac3 100644 --- a/packages/react-dom/src/__tests__/ReactDOMSelect-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMSelect-test.js @@ -362,6 +362,32 @@ describe('ReactDOMSelect', () => { expect(node.options[2].selected).toBe(true); // gorilla }); + it('does not select an item when size is initially set to greater than 1', () => { + const stub = ( + + ); + const container = document.createElement('div'); + const select = ReactDOM.render(stub, container); + + expect(select.options[0].selected).toBe(false); + expect(select.options[1].selected).toBe(false); + expect(select.options[2].selected).toBe(false); + + // Note: There is an inconsistency between JSDOM and Chrome where + // Chrome reports an empty string when no value is selected for a + // single-select with a size greater than 0. JSDOM reports the first + // value + // + // This assertion exists only for clarity of JSDOM behavior: + expect(select.value).toBe('monkey'); // "" in Chrome + // Despite this, the selection index is correct: + expect(select.selectedIndex).toBe(-1); + }); + it('should remember value when switching to uncontrolled', () => { let stub = (