Fixes issue where searchbar is not editable on chrome

[gtoonstra]

Summary of Changes

On Linux Chrome 74.0.3729.131 I was unable to edit anything in the search bar input field. I did some investigation and found that the "render" method in react always calls "getDerivedStateFromProps" before every render.

I inserted some console.log statements to confirm the flow of function calls and noticed that this was the case. The following page from react states useful things about unconditionally copying state from the props into the value:

https://reactjs.org/blog/2018/06/07/you-probably-dont-need-derived-state.html

Because of type checking I was unable to add another property to the state that was not already in props. So I opted for a simple searchTerm conditional check.

Recompiling and rerunning this on my computer gave me the ability to type text in there.

I'm by no means a web let alone a react developer, but this PR should serve as useful input to a discussion to fix this in a better way.

Tests

• The unit tests still pass
• When the page is loaded without "searchTerm" in the url, the default placeholder is inserted
• When a url is refreshed that has "searchTerm", the text is inserted into the box.

Checklist

I attempted to run "npm run lint", but that gave me errors:

Tried to lint /data/amundsen/amundsenfrontendlibrary/amundsen_application/static/jest.config.js but found no valid, enabled rules for this file type and file path in the resolved configuration.
Tried to lint /data/amundsen/amundsenfrontendlibrary/amundsen_application/static/coverage/lcov-report/prettify.js but found no valid, enabled rules for this file type and file path in the resolved configuration.
Tried to lint /data/amundsen/amundsenfrontendlibrary/amundsen_application/static/coverage/lcov-report/sorter.js but found no valid, enabled rules for this file type and file path in the resolved configuration.
Tried to lint /data/amundsen/amundsenfrontendlibrary/amundsen_application/static/dist/main.js but found no valid, enabled rules for this file type and file path in the resolved configuration.
Tried to lint /data/amundsen/amundsenfrontendlibrary/amundsen_application/static/dist/vendors.js but found no valid, enabled rules for this file type and file path in the resolved configuration.

[codecov-io]

Codecov Report

Merging #140 into master will increase coverage by 0.03%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #140      +/-   ##
==========================================
+ Coverage   53.13%   53.16%   +0.03%     
==========================================
  Files         106      106              
  Lines        2650     2652       +2     
  Branches      273      274       +1     
==========================================
+ Hits         1408     1410       +2     
  Misses       1213     1213              
  Partials       29       29

Impacted Files	Coverage Δ
...tatic/js/components/SearchPage/SearchBar/index.tsx	100% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bf707fc...9dc384f. Read the comment docs.

[gtoonstra]

I think this may need some more work, I'm still getting an uneditable search bar at times.

[whazor]

I would propose making state.searchTerm default null or false, then an if statement checking wether state.searchTerm has been set.. otherwise use props.searchTerm.

[ttannis]

@gtoonstra

This probably worth us officially stating somewhere, but the current code in this repo can reasonably be guaranteed to work in MacOS Chrome (what we develop on at Lyft, with functionality working as expected in production), so it's new to us that this bug exists. As a result in addition to updating code I'm curious to see if we can try to fully understand what is occurring differently in our environments.

I unfortunately don't have a Linux machine to try this out on. The behavior I see when typing a single character into the SearchBar is as follows:

• handleValueChange() is called, which updates the component state with the input
• render() is called, and the new value I typed is shown
  Note that I actually don't see getDerivedStateFromProps getting called. I had noticed this before and already have read the linked React doc previously as well, however at the moment I chalked up this discrepancy to the fact that the doc was for React 16.4 whereas we are on 16.3.2. Looks like we should have investigated further.

What do you observe in your environment? Are you perhaps seeing handleValueChange -> getDerivedStateFromProps -> render?

[gtoonstra]

Yes, as per the docs, getDerivedStateFromProps is now called every time before render. This is the observed flow from console.log statements. You can see my keypresses in handleValueChange:

handleValueChange: (event.target as HTMLInputElement).value=a
index.tsx:50 getDerivedStateFromProps: ret searchTerm=
index.tsx:99 render: state.searchTerm=
index.tsx:57 handleValueChange: (event.target as HTMLInputElement).value=b
index.tsx:50 getDerivedStateFromProps: ret searchTerm=
index.tsx:99 render: state.searchTerm=
index.tsx:57 handleValueChange: (event.target as HTMLInputElement).value=c
index.tsx:50 getDerivedStateFromProps: ret searchTerm=
index.tsx:99 render: state.searchTerm=
index.tsx:57 handleValueChange: (event.target as HTMLInputElement).value=d
index.tsx:50 getDerivedStateFromProps: ret searchTerm=
index.tsx:99 render: state.searchTerm=

[gtoonstra]

There are other places too. When I edit the owners of a table, I see the edit box, but after clicking "Add" the list does not build up, so clicking Save saves nothing as the array is empty. Here are all places I found the static method, but I did not test them all:

components/TableDetail/index.tsx
components/TableDetail/OwnerEditor/index.tsx
components/TableDetail/DataPreviewButton/index.tsx
components/BrowsePage/index.tsx
components/Tags/TagInput/index.tsx
components/SearchPage/SearchBar/index.tsx
components/common/EditableText/index.tsx

[gtoonstra]

Just updated the PR to use an empty string and only apply the props if state is not an empty string. This probably needs further discussion, as I'm not sure this is the right thing to do.

[gtoonstra]

It almost worked, but when everything is deleted (backspace), the value now reverts to the property setting, so you can't clear the search field entirely. Setting searchTerm to "null" didn't work for me because it failed the unit test.

[ttannis]

@gtoonstra Thanks for the extra information! Yes, getDerivedStateFromProps is documented to get called every time before render however that is applicable starting with React v16.4. Amundsen is running React v16.3.2, which is why I'm focusing on understanding this discrepancy first.

Reference: https://reactjs.org/blog/2018/05/23/react-v-16-4.html#bugfix-for-getderivedstatefromprops

The behavior of getDerivedStateFromProps that I am experiencing aligns with the expected behavior of v16.3:

Previously, it was only called if the component was re-rendered by its parent, and would not fire as the result of a local setState

The behavior of getDerivedStateFromProps that you are experiencing aligns with the expected behavior of v16.4:

getDerivedStateFromProps is now called every time a component is rendered, regardless of the cause of the update.

Next Steps:

1. I'd like to confirm what version of React you have installed in your environment. What do you get when you run npm list react --depth=0 in you environment in your .../amundsen_application/static directory. If it reports 16.3.2 -- then I'm actually still at a loss and perhaps lets skip to the next step.
2. Regardless of what's causing this discrepancy there's no denying that we have to address our usage of getDerivedStateFromProps if we ever want to bump up to 16.4. I believe returning null by default is the correct thing to do in getDerivedStateFromProps. Which unit test failed? Depending on which test failed, the correct thing to do might actually be to update the test as well.

[gtoonstra]

@ttannis : [email protected] /data/amundsen/amundsenfrontendlibrary/amundsen_application/static
└── [email protected]
Ermm.. wow! 🔢 I'm not a web dev, so don't use npm / node often. I thought these versions should always be pinned?
It's one test that checks for state being '' . I'll update the PR also updating the test.

[gtoonstra]

When I set searchTerm initialized to null, all unit tests broke because it seems to expect a string there. What I ended up doing was use another state variable to indicate if the state was changed by the user. That way, searchTerm doesn't have a magic value that changes the logic and the purpose is more explicit. Let me know what you think. There's at least another issue with the UI when editing the owners where a similar approach could be adopted.

[ttannis]

@gtoonstra Ah ha. So you are running at 5 minor versions above what we're currently developing for. React is set at 16.3.2 in our package-lock.json. This file ensures consistent installs of all of dependencies unless package.json is updated. Off the top of my head I'm unsure how you ended up with 16.8.6. Regardless, here is how I suggest we move forward:

1. Run React 16.3.2. If you delete your amundsen_application/static/node_modules folder (it is .gitignore'd you may not see it in your editor) and run npm install in amundsen_application/static, you should end up with 16.3.2. You can verify with npm list react --depth=0. If you do not, please let me know.
2. We were aware that some of our components were misusing getDerivedStateFromProps and that we needed to update some usages where the logic was unnecessary. However, at least I myself was unaware that a change between minor versions modified the side effects of getDerivedStateFromProps itself. We could either close this PR if you're able to use 16.3.2, or invest into upgrading to the React version.
   • Upgrading to the latest would require going through the past year of React's changelog and understanding which changes affect the current code.
   • Alternatively we could just take one step towards that task and upgrade to 16.4.x. I took a quick look at just the logs from 16.3.2 -> 16.4.2 and only see one other change that we'd want to check out. Besides that, the main criteria for going to 16.4.x appears to be updating all usages of getDerivedStateFromProps. We can move forward with that in this PR if desired, or we can file an issue for the community or for our team to prioritize accordingly.

[gtoonstra]

I still get 16.8.6. I verified that package and package-lock are the versions as they exist on master. This looks like an issue of my ubuntu machine that I need to resolve myself. (npm uses version 3.5.2 and node 8.10.0).

I'd recommend upgrading react with a local group of devs and look at it with fresh eyes on what the best way is to upgrade.

So I'm in favor of closing this PR, I need to sort out my machine and if that doesn't work, I can use a modified version for the time being to make things work.

[ttannis]

Sounds good. If you encounter anymore problems or might want help sorting out your machine, feel free to reach out in our slack channel. Off the top of my head you should upgrade to at least npm 6.x.x. package-lock.json was introduced in npm 5, so your npm version is not considering it.