Select and filtering doesn't apply the filter when giving a different array

[FBerthelot]

Hello ✋

Describe the bug
When sending an new array to the component, the search doesn't work.

You can see it on this screenshot:

To Reproduce
You can reproce the bug here :

https://codesandbox.io/embed/hardcore-hypatia-l5h8l?fontsize=14&hidenavigation=1&theme=dark

All you have to do is to filter in the search input, then select an item. All elements is now displayed in the dropdown even if the search input has still a value.

Expected behavior
When selecting a an element in the dropdown, the elements still need to be filtered.

Possible solution
When dropdown-tree-select are in a "controlled" mode, it should be possible to define the search programatically.
So maybe the solution is to add a props ? 🤔

Thank you 😃

[mrchief]

The issue lies in this block:

  const handleChange = () => {
    setTree(dataWithOneSelected);
  };

The component handles its internal state and maintains selection states internally. You don't need to handle onChange event unless you want to act on those selections, outside of the component - say updating state in your app's redux store or doing something based on that data with another component on the page or making an API call to do something with the data.

Your initial dataset only has those 4 nodes and onChange, you're completely replacing the initial dataset with a new one which has more nodes.

I'm not sure what your intent or use case is here but maybe we can talk about that and see how this control can help you with your requirements?

[FBerthelot]

Yes exactly, the issue come from this block. I wanted to reproduce my use case.

I have a project that is using Formik to have some helper about form validation and so on (It will be the same issue if I use some redux-form or something like that).
As the selected value are comming from an external source (formik, redux, ...), I generate a new tree array each time I render the component with react-dropdown-tree-select in it.

Many component library make the difference between controlled vs uncontrolled component even react document it.

I think, as you build a component we can consider as an input for component's user, it should be possible to have both solution: controlled and uncontrolled. In this way, even if I have a different tree each time I render, I won't be an issue as I also handle other data (controlled).

In order to achieve that, we can do multiple things:

• have a way to fully control the search input by exposing props. With this solution, user can, for example, reset search manually after select element.
• handle full tree changes

I hope it's enough clear with my english.
Let me know If I can add more precision :)

[mrchief]

This is a complex control and not a simple text or checkbox or radio field, so controlled vs uncontrolled doesn't really fit here. I've used this component with Redux-Form and formik should work just the same.

As the selected value are coming from an external source (formik, redux, ...),

Is the issue about rendering the tree with pre-selected values? You can do so by setting props in the input data.

I generate a new tree array each time I render the component with react-dropdown-tree-select in it.

Again, why? If new data comes in, you can reassign data prop and the tree will reinitialize. But you don't need to reassign the data back to the tree on every event that bubbles up.

[FBerthelot]

This is a complex control and not a simple text or checkbox or radio field, so controlled vs uncontrolled doesn't really fit here. I've used this component with Redux-Form and formik should work just the same.

Even complex control is composed of simple components. As react-dropdown-tree-select expose an API as simple as a simple input, I believe controlled vs uncontrolled fit well here. A simple API to make complexe things :)

Is the issue about rendering the tree with pre-selected values? You can do so by setting props in the input data.

Yes, it is. I use data props.

Again, why? If new data comes in, you can reassign data prop and the tree will reinitialize. But you don't need to reassign the data back to the tree on every event that bubbles up.

I've made another (more realistic) example closer to my use case : https://codesandbox.io/s/kind-minsky-x66ku?fontsize=14&hidenavigation=1&theme=dark

[mrchief]

@FBerthelot thanks for taking the time to make the detailed CodeSandbox. After going over it carefully, I think I have an idea of why you're running into the issue.

1st, upon first render when you receive data, you want to update the selected nodes. The selected values come from 2 places - server and formik's state. I think initially, formik's state will be empty and if that's the case, you can change this

const tree = adaptData(dataFromServer, selectedValuesFromFormik)

to

const tree = adaptData(dataFromServer)

and that should fix the issue.

My assumption is based on this block and the comment that describes what's going on here:

  // Adapt data (combine graphql data for tree + formik data for initial selectedValues)
  const tree = adaptData(dataFromServer, selectedValuesFromFormik);

2nd, you want to update formik's state onChange, to keep it in sync, correct?

The problem is this block being inside your main component - App in this case

  const handleChange = (_, selectedValues) => {
    setSeletedValue(selectedValues.map(val => val.label));
  };

This one updates selectedValuesFromFormik, and your tree updates due to

const tree = adaptData(dataFromServer, selectedValuesFromFormik)

but this one is unnecessary since server data hasn't changed and the dropdown doesn't need to adapt from selectedValuesFromFormik because you are sending updates from the component to formik.

Currently, because tree is dependent on both, React re-renders App and that is re-rendering the dropdown as well.

The thing is, updating formik's state should not cause the App to re-render, unless there are other components that you need to re-render. In any case, there is no reason to re-render the Dropdown since the dropdown is tracking it's state already.

And if your actual use cases requires you to update selectedValuesFromFormik, you can do so in your handleChange but there is no reason to assign it back to the component.
Unless, there are other components updating formik state and you need to update dropdown based on those selections. If that's so, then there are few ways of solving this but let me know first. I don't want to complicate this post if there's no reason to.

This is a long one and please let me know if anything doesn't make sense.

[FBerthelot]

Thank you a lot for taking time to answer with this detailed answer :)

1st: Yes exactly, initial values are coming from a request and tree data are coming from another request. So, I have to combine both data in order to build the tree.

I've thought about making the tree data request earlier but it's a complex form and it's not great solution (user can toggle the dropdown tree field).

2nd: Yes, I need to update formik state each time the selected value are updated. Otherwise, form submit won't work as expected, validation also won't work.

The thing is, updating formik's state should not cause the App to re-render, unless there are other components that you need to re-render. In any case, there is no reason to re-render the Dropdown since the dropdown is tracking it's state already.

I totally agree with that point ! It can cause bad performances on big form even if I use Fromik fast field for performance. But in this specific case I cannot blame formik, as I understand a re-render occur when value of the field is changing. Formik and those kind of library expect to work with controlled component, formik is the source of truth about current value and we need to wire the field event with formik callback (double way data binding).

In my case, I've though to implement a little cache with useEffect. Code Sandbox here.
But the same issue occur as formik would re-render the component.

I've also try isolate the dropdown component with a react.memo, but it doesn't work :/

So to answer your final question, yeah formik re-render a lot of stuff when a single value change just for validation. I need it in my use case, but in the other hand it's not good for performance and it can cause issue like this.

Again, thank you for taking time to answer. It's very kind :)

[mrchief]

I made an updated sandbox from prevent-re-render-on-parent-render-hoc topic which may help: https://codesandbox.io/s/pedantic-tdd-ofwzz

Haven't had the time to hookify the HoC but I think you can adapt useEffect or use something like https://github.com/kentcdodds/use-deep-compare-effect to achieve the same. If you do, let me know and maybe we can add another help topic to the docs. :)

I've used the HoC approach with redux-forms so I guess it should work with formik as well.

[mrchief]

I totally agree with that point ! It can cause bad performances on big form even if I use Fromik fast field for performance. But in this specific case I cannot blame formik, as I understand a re-render occur when value of the field is changing. Formik and those kind of library expect to work with controlled component, formik is the source of truth about current value and we need to wire the field event with formik callback (double way data binding).

It's not Formik's fault, it's just how React works fundamentally. I think you should try hoisting (like the HoC example does) your form or components to avoid re-render when you update Formik state. This is a common and well known problem. Re-rendering is not an issue for simple components like text boxes et. al. but for complex forms with lots of components re-rendering should be taken into consideration. No one likes a flashing page. :)

[FBerthelot]

This example : https://codesandbox.io/s/pedantic-tdd-ofwzz is also buggy, exactly like my example. In this example, there is no HOC but there is a container for the field, like in your prevent-re-render-on-parent-render example.

The approach you are using is like what I've made with the react.memo: try to have another component in order to avoid a re-render.

I've updated my previous example with the use of useMemo from react. I've implemented something ridiculously complex with useEffect as it's already exist in react...

I've made something work with a class component and shouldComponentUpdate that always return false. https://codesandbox.io/s/aged-field-uxkrf?fontsize=14&hidenavigation=1&theme=dark
In the other hand, I have trouble to do this with React.memo.

I have to go, I will continue investigation on it tomorrow.

Count on me for adding a documentation on it when we will find the solution 💪

Bye :)

[mrchief]

The approach you are using is like what I've made with the react.memo: try to have another component in order to avoid a re-render.

This is known as hoisting and is a recommended pattern, not a hacky solution. :)

The problem with memo is that it does an Object.is check, and a not a deep equal. For deep equal, you'd need to use useEffect or the use-deep-compare-effect package. But then again, I don't see why everyone is so obsessed with ditching classes completely. I can see a reason to do away with deprecated lifecyle events but I don't understand why everything has to be a hook. In other words, the HoC can remain a class and that is fine IMO, but if you're willing to figure out a hook based solution then I wouldn't stop you. :)

[FBerthelot]

Hey, I'm back :)

Yes, It's Hoisting is a great and elegant solution, thank you for giving me the right name :)

I see two way to handle data change in your component API.

• Each time a change is made on data, re-render the whole component. React will to the optimization with some of the data haven't change (key with id are important in this case with react.memo and simple props). This will may cause a CPU pick each time data are changing.

• Disable watch on data props change, a just initial data are relevant. InitialData are copy/pasted (the copy of the reference can be enough). This may cause a higher usage of memory in case of deep objet copy or it can cause this weird effect (like this one) when copy reference.

If you want more literature about this.

You chose the second solution with a reference copy. So let's help user consider data shouldn't be changed frequently as it can cause bugs.

How can we improve the documentation on this ?

I've made an example on Hoisting with classes and shouldComponentUpdate (your example actually), and another with hooks.

https://codesandbox.io/s/aged-field-uxkrf?fontsize=14&hidenavigation=1&theme=dark

In the example, I don't use any costly deep-object comparison algorithm.
I've just forgot to memoize the handleChange function to make this work before.

Hope this can help anyone some day :)
Have a great day

[mrchief]

Each time a change is made on data, re-render the whole component. React will to the optimization with some of the data haven't change (key with id are important in this case with react.memo and simple props). This will may cause a CPU pick each time data are changing.

Forgive me for not relying on React to do the optimization :). There is a great deal of nuances to that and it's not as simple. Besides, you also have to bear the cost of initial tree parsing, lose out on search terms caching, forego any optimizations/caching infinite scroll component does and so on.

Disable watch on data props change, a just initial data are relevant. InitialData are copy/pasted (the copy of the reference can be enough). This may cause a higher usage of memory in case of deep objet copy or it can cause this weird effect (like this one) when copy reference.

I'm not sure I follow.

How can we improve the documentation on this ?

I'll add a hook based HoC example to the docs, so thanks for sending the CodeSandbox.

In the example, I don't use any costly deep-object comparison algorithm.

That is left as a choice to the user and they can do so in their own container pretty easily.

[FBerthelot]

There is nothing to forgive ;) It's only a choice, each strategy have is own pro and cons.

Thank you for the time you took to answer me.

I let you close the issue when you think it's good.

Let me know if you need some other code sandbox :)

Bye,