[Autocomplete] componentWillReceiveProps method's condition

[sameeri]

The condition in the componentWillReceiveProps currently compares this.props.searchText and nextProps.

If, on each re-render, we want to have a new value for searchText, like ' ', for example, then we need to compare the nextProps.searchText to this.state.searchText or have no comparison at all & just set it to the passed in value.

Could you please help me understand the behavior for this component. Is this a desired effect? Or is this a bug?

Thank you!

[oliviertassinari]

I don't agree.
Let's say we implement what you have suggested. Then, if we type on a key inside the TextField and a render occurs, the searchText will be set to the original property. That's not what we want.

Can you use onUpdateInput to upadate the searchText property?

[sameeri]

What is the intention of searchText? If it is an input to the component, it should not be mutated.
I had modified the code inside the node module as i had described and i don't see any negative effects.

[oliviertassinari]

The searchText property is here to let you change the search text.

I had modified the code inside the node module as i had described and i don't see any negative effects.

Could we see this change? That will help.

[sameeri]

@oliviertassinari Will do. Today. Thank you so much for being so kind and so patient. I apologize for not being prompt. Deadlines and stuff have taken priority.

I would like to add that Material UI is a great library of components. And so far it has been great to work with them and i have been learning a lot! 👏

[sameeri]

@oliviertassinari

Here are the code changes that i made in my node_modules. I just removed the inequality check, in the componentWillReceiveProps method and setState each time with the searchText property supplied.

https://github.com/sameeri/material-ui/blob/master/src/auto-complete.jsx

Here is the context:

Architectural setup => Flux + React + react-router

Principles =>
https://medium.com/@dan_abramov/smart-and-dumb-components-7ca2f9a7c7d0#.snru4z36s

So let's say the app has a set of super heroes and based on some super hero searched via the Auto Complete component, we display the superhero's characteristics, maybe using our Card component.

And upon some particular user actions on the page, there is a necessity of starting with a completely blank slate. That includes the clear of our Auto Complete's searchText.

Since we are using the container + presentation component separation and data flows into the component. I expected the Auto Complete component to honor the searchText input property and clear the searchText, upon re-rendering. Just like any other component. This is however not the behavior of the component.

So i dug into the code. And these are my notes from the code & the observed behavior.

• There is an input property called 'searchText'.
• There is a corresponding state property called 'searchText'.
• In the initial state, searchText is set based on this.props
• And any user input updates the searchText accordingly.
• The search algorithm fires and user is provided with options as a drop down menu.
• Any user actions, like selecting a menu item from the drop down or hitting an enter will update the state's searchText.
• Upon re-render, there is a comparison of nextProps.searchText and this thisProps.searchText for inequality and the state is set if the props change.

Since i am treating it as a presentation component, the props will always be the same, the current code sees the this.props and nextProps as the same thing and it won't honor the property specified and state will not update.

The method you have specified 'onUpdateInput ' fires on each key stroke if i remember the behavior correctly. So i don't know if it is a suitable method.

To better understand my use of components, and predict the behavior of the component, i need to understand the component's intentions. I had made changes, and it solves my problem, but i want to get an insight into the design of the component.

I do not have the example readily setup to demonstrate my use case.

Is there any way to do what i have described?

Thank you so much for your time!!

[oliviertassinari]

@sameeri Let me ask you a single question. How would you solve this clearing use case with a regular <input /> component?

[sameeri]

That's a great question @oliviertassinari.

I was wondering the same. The whole confusion lies with that principle concept. If we treat the AutoComplete as an input component, then i think we should just not make equality comparison between the old value and the new value.

If the component honors inputs, I should not be doing anything special. If i want to set the value of an input, i should just pass it along.

For example, the TextField component honors the value property passed to it and in componentWillReceiveProps and sets the state based on nextProps.value.

https://github.com/sameeri/material-ui/blob/master/src/TextField/TextField.jsx

This is the same behavior i would expect with Autcomplete(treating it as input)

I am gauranteed that the regular input or the TextField treats the value property as is. And if i want to change it, i pass it a new value.

[sameeri]

We can take this discussion to some place better if this is not the right place. I am sorry, it got so long.

[oliviertassinari]

This is the same behavior i would expect with Autcomplete(treating it as input)

That's a very good point. Using the same logic as the TextField sounds like a good idea 👍.That a small breaking changes, but as we are going to release v0.15 that should be fine.

[sameeri]

Thank you so much @oliviertassinari. Then, the AutoComplete would behave as i expect :) 👍
You have been so patient with all my questions. I sincerely appreciate that. I am just trying to solidify my understanding of Components and Component design. Its more of a "how should i think about this" and since Material UI is outstanding as a suite of components, i am taking it as my reference.

[oliviertassinari]

@sameeri Thanks for raising this 👍. Do you want to submit a PR?

[sameeri]

Hey @oliviertassinari I'm so sorry i missed this. Sure, would love to. What's the procedure?

[sameeri]

I am going through the https://github.com/callemall/material-ui/blob/master/CONTRIBUTING.md

I will create the PR as soon as i can. Thank you so much for the opportunity. :) 👍

[nathanmarks]

@sameeri This was tagged incorrectly, sorry.

[oliviertassinari]

For example, the TextField component honors the value property passed to it and in componentWillReceiveProps and sets the state based on nextProps.value.

I just had a look at the TextField implementation and that's not the true from what I can see.

Sorry, but I can't see better way to do it.
I think that your PR would introduce a regression. The AutoComplete can be uncontolled. Any rerender will reset the search field.

[sameeri]

@oliviertassinari
Its fine. I do not understand how it would, i will take a look at this again. Thank you!

[paol]

@oliviertassinari I've just run into this problem and it's not just an enhancement, it's a bug. The problem is that AutoComplete does not behave like a controlled component.

For example:

1. We start with searchText=""
2. The user select an item from the dropdown (let's say the text "Foo"). The value of the text field is now "Foo" but onUpdateInput is not fired. onNewRequest however is fired.
3. In the handler for onNewRequest we want to clear the text field, so we set searchText="" again. But AutoComplete thinks that nothing has changed because searchText was already an empty string before, so it ignores the prop.

The problem already started at 2) because inside the AutoComplete component, the values of state.searchText and props.searchText have become out of sync. At 3) we're left with no way to clear the text field, because the condition in componentWillReceiveProps prevents it.

I only see 2 solutions:
A - Always fire onUpdateInput before onNewRequest so that the controller is responsible for setting searchText to the new value, like you would expect for a controlled component.
B - Be smarter in componentWillReceiveProps to detect these situations. It should be possible to distinguish controlled vs. uncontrolled scenarios and act accordingly.

I'm willing to do a PR to fix this but I'm not sure which solution to pick, both seem kind of fragile.

[oliviertassinari]

@paol Right, thanks for providing those details. I think that we should be as simple as possible. Option A sounds like the best option here 👍.

[mykyta-shulipa]

Prerequisites:
a. Autocomplete options is coming from server based on what user type.
b. Inside of onUpdateInput, system make a call to server to get available data.

Scenario

1. User entered some text into autocomplete field (callback onUpdateInput was fired )
2. Data is coming from server, available options shown.
3. User select one of available item (callback onUpdateInput was fired - so additional and useless call to server was made, then onNewRequest callback was fired)

Question
is there any way to determine inside of onUpdateInput that it was user selection, and not new input?
I can't handle it by myself because onUpdateInput was executed earlier then onNewRequest...

[oliviertassinari]

is there any way to determine inside of onUpdateInput that it was user selection, and not new input?

@nktssh Not I'm aware of. I feel like caching the network answer of the first onUpdateInput called could be a good workaround.

[paol]

Caching isn't that straightforward, the callee would need to distinguish when to use the cache and when to make a new request, which brings us back to the original problem.

Option 1: Add another parameter to the onUpdateInput callback, to make the distinction possible.
Option 2: Add another callback to signal "this is the right time to update the option list", only fired when the user types into field.

[oliviertassinari]

I have another solution in mind. What if we stop mutation the TextField value when the AutoComplete is controlled and we revert #5518?
I think that it would solve all our issues in a elegant way.

[paol]

Not sure what you mean by stop mutating the TextField, could you explain?

[oliviertassinari]

Not sure what you mean by stop mutating the TextField, could you explain?

I think that those lines should be the responsibility of the parent component when controlling the AutoComplete. Basically, I would remove all unpredictable mutations.

[paol]

Let me see if I understand: you mean remove state.searchText from AutoComplete entirely, and wire props.searchText directly to the TextField value, and presumably wire the TextField onChange to the AutoComplete onUpdateInput.

If that can be done then great but it would be a breaking change, no?

[oliviertassinari]

Let me see if I understand

Yeap, that's basically what I'm suggesting. Right, that could be considered a breaking change.
I think that it will only make into the next branch. This AutoComplete needs quite some improvements!

[paol]

Well if you make breaking changes may I humbly suggest renaming some props?

searchText -> value
onUpdateInput -> onChange
onNewRequest -> onItemSelect or onOptionSelect

The first 2 would be a lot more consistent with other controls, and the last would be more intuitive.

[oliviertassinari]

@paol I agree with the props renames 👍
We have this issue regarding moving forward #5062.
We have already made a countless number of breaking changes on the next branch.