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

Conversation

vursen
Copy link
Contributor

@vursen vursen commented Nov 25, 2024

Description

Reassigning a sync property to the same value should not trigger a Lit re-render.

Fixes #8221

Type of change

  • Bugfix

@vursen vursen marked this pull request as ready for review November 25, 2024 11:59
@vursen vursen marked this pull request as draft November 25, 2024 12:05
@vursen vursen force-pushed the fix/do-not-re-render-when-property-has-not-changed branch from 66e84db to 9e08816 Compare November 25, 2024 12:09
@@ -105,7 +106,7 @@ const PolylitMixinImplementation = (superclass) => {
this.requestUpdate(name, oldValue, options);

// Enforce synchronous update
if (this.hasUpdated) {
if (notEqual(oldValue, value) && this.hasUpdated) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

note: Lit supports customizing property change detection with hasChanged. However, I decided that adding it to PolylitMixin is not useful since Polymer doesn't support this feature anyway.

@vursen
Copy link
Contributor Author

vursen commented Nov 25, 2024

Some of the failures make sense – they reveal that certain properties lack sync: true, for example:

class OverlayMixin extends OverlayFocusMixin(OverlayStackMixin(superClass)) {
static get properties() {
return {
/**
* When true, the overlay is visible and attached to body.
*/
opened: {
type: Boolean,
notify: true,
observer: '_openedChanged',
reflectToAttribute: true,
},

@vursen vursen force-pushed the fix/do-not-re-render-when-property-has-not-changed branch from 18569ff to f81d99a Compare November 25, 2024 13:28
@vursen vursen force-pushed the fix/do-not-re-render-when-property-has-not-changed branch from 1616a5e to c1f4599 Compare November 25, 2024 17:05
@vursen vursen force-pushed the fix/do-not-re-render-when-property-has-not-changed branch from 3995a63 to 08ba456 Compare November 26, 2024 09:56
@vursen vursen marked this pull request as ready for review December 6, 2024 14:06
@vursen vursen requested a review from web-padawan December 6, 2024 14:06
Copy link

sonarqubecloud bot commented Dec 6, 2024

Comment on lines +11 to +13
if (!entry.target.isConnected) {
return;
}
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.

@vursen vursen merged commit 4b7c160 into main Dec 12, 2024
9 checks passed
@vursen vursen deleted the fix/do-not-re-render-when-property-has-not-changed branch December 12, 2024 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[PolylitMixin] Should not trigger update when assigning the same value to a sync property
3 participants