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.

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

fix: do not re-render when setting sync property to same value #8223

Merged
merged 14 commits into from
Dec 12, 2024
22 changes: 11 additions & 11 deletions packages/component-base/src/polylit-mixin.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
* This program is available under Apache License Version 2.0, available at https://vaadin.com/license/
*/
import { dedupeMixin } from '@open-wc/dedupe-mixin';
import { notEqual } from 'lit';
import { get, set } from './path-utils.js';

const caseMap = {};
Expand Down Expand Up @@ -101,12 +102,15 @@ const PolylitMixinImplementation = (superclass) => {
get: defaultDescriptor.get,
set(value) {
const oldValue = this[name];
this[key] = value;
this.requestUpdate(name, oldValue, options);

// Enforce synchronous update
if (this.hasUpdated) {
web-padawan marked this conversation as resolved.
Show resolved Hide resolved
this.performUpdate();
if (notEqual(value, oldValue)) {
this[key] = value;
this.requestUpdate(name, oldValue, options);

// Enforce synchronous update
if (this.hasUpdated) {
this.performUpdate();
}
}
},
configurable: true,
Expand All @@ -115,21 +119,17 @@ const PolylitMixinImplementation = (superclass) => {
}

if (options.readOnly) {
const setter = defaultDescriptor.set;
const setter = result.set;

this.addCheckedInitializer((instance) => {
// This is run during construction of the element
instance[`_set${upper(name)}`] = function (value) {
setter.call(instance, value);

if (options.sync) {
this.performUpdate();
}
};
});

result = {
get: defaultDescriptor.get,
get: result.get,
set() {
// Do nothing, property is read-only.
},
Expand Down
4 changes: 4 additions & 0 deletions packages/component-base/src/resize-mixin.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@ import { dedupingMixin } from '@polymer/polymer/lib/utils/mixin.js';
const observer = new ResizeObserver((entries) => {
setTimeout(() => {
entries.forEach((entry) => {
if (!entry.target.isConnected) {
return;
}
Comment on lines +11 to +13
Copy link
Member

Choose a reason for hiding this comment

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

Is this change related? Couldn't find which package is affected

Copy link
Contributor Author

@vursen vursen Dec 12, 2024

Choose a reason for hiding this comment

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

As far as I noticed, some tests finish before the resize observer processes the changes they triggered (at least setTimeout adds a delay). If the observer does something that triggers a Lit update, Lit throws an exception because the element no longer exists by that moment. This exception only appears in the logs and doesn't cause a failure because the test has already finished. Let me know if you would like me to exclude this change from the scope of this PR.


// Notify child resizables, if any
if (entry.target.resizables) {
entry.target.resizables.forEach((resizable) => {
Expand Down
14 changes: 14 additions & 0 deletions packages/component-base/test/polylit-mixin.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1079,6 +1079,20 @@ describe('PolylitMixin', () => {
expect(element.hasAttribute('helper')).to.be.true;
});

it('should not reflect immediately when setting a read-only sync property to the same value', () => {
element._setOpened(true);
element._setHelper('foo'); // async property
element._setOpened(true); // sync property
expect(element.hasAttribute('helper')).to.be.false;
});

it('should not reflect immediately when setting sync property to the same value', () => {
element.disabled = true;
element._setHelper('foo'); // async property
element.disabled = true; // sync property
expect(element.hasAttribute('helper')).to.be.false;
});

it('should only call ready callback once during initialization', () => {
expect(element.count).to.equal(1);
});
Expand Down
5 changes: 1 addition & 4 deletions packages/date-picker/src/vaadin-date-picker-mixin.js
Original file line number Diff line number Diff line change
Expand Up @@ -1211,10 +1211,7 @@ export const DatePickerMixin = (subclass) =>
if (!dateEquals(this.__enteredDate, date)) {
this.__enteredDate = date;
}
} else if (this.__enteredDate != null) {
// Do not override initial undefined value with null
// to avoid triggering a Lit update that can cause
// other scheduled properties to flush too early.
} else {
this.__enteredDate = null;
}
}
Expand Down
1 change: 1 addition & 0 deletions packages/field-base/src/input-mixin.js
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ export const InputMixin = dedupingMixin(
type: Boolean,
value: false,
observer: '_hasInputValueChanged',
sync: true,
},
};
}
Expand Down
1 change: 1 addition & 0 deletions packages/grid/src/vaadin-grid-column-group-mixin.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ export const GridColumnGroupMixin = (superClass) =>
width: {
type: String,
readOnly: true,
sync: true,
},

/** @private */
Expand Down
10 changes: 8 additions & 2 deletions packages/grid/src/vaadin-grid-column-mixin.js
Original file line number Diff line number Diff line change
Expand Up @@ -177,10 +177,16 @@ export const ColumnBaseMixin = (superClass) =>
_emptyCells: Array,

/** @private */
_headerCell: Object,
_headerCell: {
type: Object,
sync: true,
},

/** @private */
_footerCell: Object,
_footerCell: {
type: Object,
sync: true,
},

/** @protected */
_grid: Object,
Expand Down
8 changes: 8 additions & 0 deletions packages/grid/src/vaadin-grid-sorter-mixin.js
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,14 @@ export const GridSorterMixin = (superClass) =>
/** @protected */
connectedCallback() {
super.connectedCallback();

if (this.performUpdate) {
web-padawan marked this conversation as resolved.
Show resolved Hide resolved
// Force Lit's first render to be synchronous to ensure
// _pathOrDirectionChanged is triggered before grid's
// __applySorters, as it is in Polymer.
this.performUpdate();
}

if (this._grid) {
this._grid.__applySorters();
} else {
Expand Down
1 change: 1 addition & 0 deletions packages/item/src/vaadin-item-mixin.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ export const ItemMixin = (superClass) =>
value: false,
reflectToAttribute: true,
observer: '_selectedChanged',
sync: true,
},

/** @private */
Expand Down
1 change: 1 addition & 0 deletions packages/select/src/vaadin-select-base-mixin.js
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@ export const SelectBaseMixin = (superClass) =>
value: '',
notify: true,
observer: '_valueChanged',
sync: true,
},

/**
Expand Down
Loading