Remove componentWillReceiveProps() in AutoSearchInput

[willdurand]

See also: #12323

---

The following patch works but one test case is still failing (see after the patch):

diff --git a/src/amo/components/AutoSearchInput/index.js b/src/amo/components/AutoSearchInput/index.js
index 2fd203486..91ad64b6d 100644
--- a/src/amo/components/AutoSearchInput/index.js
+++ b/src/amo/components/AutoSearchInput/index.js
@@ -109,6 +109,17 @@ export class AutoSearchInputBase extends React.Component<InternalProps, State> {
     showInputLabel: true,
   };
 
+  static getDerivedStateFromProps(props: InternalProps, state: State) {
+    if (props.query !== state.searchValue) {
+      return {
+        ...state,
+        searchValue: props.query || '',
+      };
+    }
+
+    return null;
+  }
+
   constructor(props: InternalProps) {
     super(props);
     invariant(props.inputName, 'The inputName property is required');
@@ -143,14 +154,6 @@ export class AutoSearchInputBase extends React.Component<InternalProps, State> {
     };
   }
 
-  componentWillReceiveProps(nextProps: InternalProps) {
-    const { query } = nextProps;
-
-    if (this.props.query !== query) {
-      this.setState({ searchValue: query || '' });
-    }
-  }
-
   createFiltersFromQuery(query: string) {
     const { location, userAgentInfo } = this.props;
     // Preserve any existing search filters.

Here is the failing test case:

● /Users/williamdurand/projects/mozilla/addons-frontend/tests/unit/amo/components/TestAutoSearchInput.js › search input › only sets an updated query if it is unique

expect(received).toMatchObject(expected)

Expected value to match object:
  {"value": "panda themes"}
Received:
  {"className": "AutoSearchInput-query", "id": "AutoSearchInput-query", "maxLength": 100, "minLength": 3, "name": "query", "onChange": [Function anonymous], "placeholder": "Find add-ons", "type": "search", "value": "ad blocker"}
Difference:
- Expected
+ Received

  Object {
-   "value": "panda themes",
+   "value": "ad blocker",
  }

  134 |       root.setProps({ query });
  135 |
> 136 |       expect(getInputProps(root)).toMatchObject({ value: typedQuery });
      |                                   ^
  137 |     });
  138 |
  139 |     it('handles typing text into the input', () => {

  at Object.<anonymous> (tests/unit/amo/components/TestAutoSearchInput.js:136:35)

From what I understand, the test case tries to cover the following scenario:

1. set a value for a query prop
2. this value is copied to the component's state in the constructor
3. the state is updated via a child component
4. new props are given to the component
5. we expect that the query prop is NOT copied to the component's state that time

I find this scenario a bit weird because we assume a state property can be updated by two different "sources", yet not always.

Is that a real life scenario?

---

For QA: please make sure the search bar still works as intended.

[kumar303]

I think the test case may be correct. If typing in the input box changes state.query then this will be out of sync with props.query. When that happens, any kind of application re-render (which might cause a re-render of AutoSearchInput with the old query prop) should not erase what the user has typed.

I would suggest investigating what kind of re-render situations could affect this. Maybe hot reloading? Maybe a URL change? In the case that you can't find any, it seems to me that this.state should still be the source of truth since it is tied to what the user has typed in the input field.

[willdurand]

Thanks a lot for your input! I hate removing test cases without a good reason.

I'll see what I can do because that seems to be slightly against the reactive model introduced by React.

[willdurand]

@kumar303 so, we actually hit the anti-pattern described here: https://reactjs.org/blog/2018/06/07/you-probably-dont-need-derived-state.html#anti-pattern-erasing-state-when-props-change

We should use the key approach I think, but I don't know which value to use yet.

[kumar303]

I looked into how query is passed to AutoSearchInput in practice. My conclusion is that AutoSearchInput is an uncontrolled component and its internal state is the source of truth.

Notes:

• The query prop is only used to set the initial value of the input field. All other changes come from what the user types. You can probably edit (or delete) that failing test accordingly.
• There is some prop drilling madness going on here; AutoSearchInput doesn't need to accept a query prop at all. It only needs to load an initial value from location.query.q, that's it. It can use withRouter for that. This might be a bigger change, though.

[willdurand]

The query prop is only used to set the initial value of the input field. All other changes come from what the user types. You can probably edit (or delete) that failing test accordingly.

👍

There is some prop drilling madness going on here; AutoSearchInput doesn't need to accept a query prop at all. It only needs to load an initial value from location.query.q, that's it. It can use withRouter for that. This might be a bigger change, though.

I think query is a string value here, and not location.query.

[willdurand]

I fixed the component.

[ioanarusiczki]

@willdurand Testing scenarios for qa in #12323 were:

QA: there should be no issue when navigating between pages (for example, when browsing multiple add-ons, refreshing a page, then navigating to some other add-ons linked on a page, etc.)

Is that what it needs to be done here too?

[willdurand]

Is that what it needs to be done here too?

oops, I forgot to update the description. You can test the search bar.

[AlexandraMoga]

Verified fixed on AMO stage with FF63, Win10x64 and Android 8.0

The search bar from the page header and from the collection edit page is working as intended.