Skip to content

Commit

Permalink
fix: do not re-render when setting sync property to the same value (#…
Browse files Browse the repository at this point in the history
  • Loading branch information
vursen authored Dec 12, 2024
1 parent eb8470d commit 4b7c160
Show file tree
Hide file tree
Showing 10 changed files with 50 additions and 17 deletions.
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) {
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;
}

// 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 @@ -1051,6 +1051,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) {
// 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

0 comments on commit 4b7c160

Please sign in to comment.