Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor: reduce item renderer calls in combo box #8173

Merged
merged 2 commits into from
Nov 20, 2024
Merged

refactor: reduce item renderer calls in combo box #8173

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e6a26ed
chore: reduce item renderer calls in combo box
sissbruecker Nov 19, 2024
a4f21f6
address review comments
sissbruecker Nov 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 10 additions & 10 deletions packages/combo-box/src/vaadin-combo-box-data-provider-mixin.js
View file
Open in desktop
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#7044 introduced a number of requestContentUpdate calls for several scenarios:

  • Clear cache
  • Change size
  • Page loads

With that, the component now effectively runs ComboBoxMixin.requestContentUpdate three times when filtering using a data provider. Previously the method wasn't run at all and only the scroller would update.

But ComboBoxMixin.requestContentUpdate, in addition to updating the scroller, also runs renderers for all items again. So that results in three additional renderer calls for each item, for each filter change, when using a data provider.

Before the change

after.mov

After the change

before.mov

It looks like we can optimize that to only run requestContentUpdate on the scroller instead. I guess the question is then if there is any logic in data provider mixin that could cause an item's data to be updated, instead of the whole item reference, in which case this optimization would probably break something.

Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ export const ComboBoxDataProviderMixin = (superClass) =>
this.__dataProviderInitialized = true;

if (this.dataProvider) {
this.requestContentUpdate();
this.synchronizeControllerState();
}
}

Expand Down Expand Up @@ -166,7 +166,7 @@ export const ComboBoxDataProviderMixin = (superClass) =>
const { rootCache } = this.__dataProviderController;
rootCache.items = [...rootCache.items];

this.requestContentUpdate();
this.synchronizeControllerState();

if (!this.opened && !this._isInputFocused()) {
this._commitValue();
Expand All @@ -183,7 +183,7 @@ export const ComboBoxDataProviderMixin = (superClass) =>

this.__dataProviderController.clearCache();

this.requestContentUpdate();
this.synchronizeControllerState();

if (this._shouldFetchData()) {
this._forceNextRequest = false;
Expand All @@ -206,7 +206,7 @@ export const ComboBoxDataProviderMixin = (superClass) =>
// The controller adds new placeholders to the cache through mutation,
// so we need to create a new array to trigger filteredItems observers.
rootCache.items = [...rootCache.items];
this.requestContentUpdate();
this.synchronizeControllerState();
}
}

Expand All @@ -224,19 +224,17 @@ export const ComboBoxDataProviderMixin = (superClass) =>
const { rootCache } = this.__dataProviderController;
if (rootCache.items !== items) {
rootCache.items = items;
this.requestContentUpdate();
this.synchronizeControllerState();
}
}
}

/**
* Synchronizes the controller's state with the component, which can be
* out of sync after the controller receives new data from the data provider
* or if the state in the controller is directly manupulated.
*
* @override
* or if the state in the controller is directly manipulated.
*/
requestContentUpdate() {
synchronizeControllerState() {
vursen marked this conversation as resolved.
Show resolved Hide resolved
// When the data provider isn't initialized, it means the content update was requested
// by an observer before the `ready()` callback. In such cases, some properties
// in the data provider controller might still be uninitialized, so it's not safe
Expand All @@ -249,7 +247,9 @@ export const ComboBoxDataProviderMixin = (superClass) =>
this.loading = this.__dataProviderController.isLoading();
}

super.requestContentUpdate();
if (this._scroller) {
this._scroller.requestContentUpdate();
}
vursen marked this conversation as resolved.
Show resolved Hide resolved
}

/** @private */
Expand Down