Fixes #1472 Fixing existing tests #1474
Conversation
…a source. Fixes liferay#1472 Fixes liferay#1472 Also removing polling. Data is now fetched on based on a query. Fixes liferay#1472 Fixes liferay#1472 At the moment the default setup performs a single query, caches the Fixes liferay#1472 results and then filters them manually. The more realistic use case is Fixes liferay#1472 to send the query typed by the user to a backend that returns related Fixes liferay#1472 results. Polling will also cause unnecessary overhead to the server.
For some reason this fails on my machine but seems to work in travis. This reverts commit f3b0da3.
|
Just started reviewing :) |
Sent from 
There was a problem hiding this comment.
hey @4lejandrito Just listing some of my concerns with this change.
|
|
||
| if (this.enableAutocomplete) { | ||
| this._handleFilteredItems(event, query); | ||
| this.refs.dataProvider.updateData(this._query); |
There was a problem hiding this comment.
hey @4lejandrito, we have to be careful for use cases that do not need a query in the URL or that do not need to make requests to each user input, in this way will always request a request unnecessarily.
| * @memberof ClayDataProvider | ||
| * @type {?(number|undefined)} | ||
| */ | ||
| requestPolling: Config.number().value(0), |
There was a problem hiding this comment.
hey @4lejandrito I understand that for some use cases this would not do much feeling, but we may have cases where you want the data to be updated every minute for example... with DataProvider we intend to use it in other places, such as ClayCharts. I would like to keep this API to cover these cases.
| @@ -18,6 +18,8 @@ class ClayAutocomplete extends ClayComponent { | |||
| * @return {Boolean} If the event has been prevented or not. | |||
| */ | |||
| _handleDataChange(event) { | |||
| this._handleFilteredItems(event); | |||
There was a problem hiding this comment.
If we always keep the call to updateData when the query changes, we will have the use case when we do not want to make a request for each query change, in this case we will always be issuing the dataChange event without them being altered
| } | ||
| this.updateData = debounce( |
There was a problem hiding this comment.
I do not know if debounce is necessary when we do not want to make a request every time the query changes.
|
Hi @matuzalemsteles, Thanks for looking into this. I was under the impression that DataProvider was only used by the autocomplete. I understand you might want to cover future use cases with it, but I think you're expecting too much from a single component. Also, autocompletion requirements are very different from those of a dynamic real time chart. The reason of our changes is that we need to query a backend service that provides autocompletion results based on the user input. The best way to provide accurate results is if we query the server every time the user stops typing for a small fraction of a second (this is what we use debounce for). If we want to support charts and autocomplete, I would personally recommend having 2 different components. This way we wouldn't have dependencies between the 2 modes of operation and we would be able to evolve both components separately. Otherwise we might end up breaking autocomplete each time we need a new feature for charts and vice-versa. I am happy to work on my changes to try to keep both modes working at the same time, but I think it is a waste of our efforts since at the moment there is no other consumer of DataProvider. What are your thoughts about this? |
|
Also, this is a good read about the different approaches of autocompletion: https://www.peterbe.com/plog/how-to-throttle-and-debounce-an-autocomplete-input-in-react. |
|
hey @4lejandrito, I understand your point of view, the DataProvider was extracted from Autocomplete initially, we separated it to start evolving to support our other components (See #1292 (comment)), initially it is being used only by Autocomplete and MultiSelect . Please, I am not disagreeing with the use of debounce or best practices to do autocomplete, instead I am thanking you for your contribution. But we need to make sure that this component is not designed for just the use case of Autocomplete, for example. My recommendation to follow these changes is to adjust the modifications to support the use case of when we do not want to make a request for each query change, we can enable this functionality by adding a new API. What do you think? Thanks! |
|
Hi @matuzalemsteles, Thanks for your response. I've had a chat with @jbalsas and they will take over this. Feel free to get back to me if I can be of any help. Regards! |
|
Just started reviewing :) |
| requestRetries === 0 | ||
| ) { | ||
| updateData(query, requestRetries = 0) { | ||
| if (this._hasData(this.dataSource)) { |
There was a problem hiding this comment.
@4lejandrito is this needed? We're already doing this in the created method.
There was a problem hiding this comment.
Hi @carloslancha, see https://github.com/carloslancha/clay/pull/8. With the last commit we can remove that if statement. Now we filter the items every time the query changes or every time the data changes.
There was a problem hiding this comment.
I think I accidentally pushed the commit to this PR too...
No description provided.