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

First render event updates appear not to trigger update #549

Closed
3 tasks done
zombiezen opened this issue Feb 6, 2019 · 4 comments · Fixed by #555
Closed
3 tasks done

First render event updates appear not to trigger update #549

zombiezen opened this issue Feb 6, 2019 · 4 comments · Fixed by #555

Comments

@zombiezen
Copy link

Description

Events that are triggered soon after (during?) the first render seem to not trigger an update.

Live Demo

https://stackblitz.com/edit/lit-element-example-xf7hpe?file=index.js

Steps to Reproduce

  1. Observe that the console logs and DOM contents.
  2. Click to make other path changes.

Expected Results

  1. Console log should show "_pathChanged has big mood: /" and the DOM should show "Web Components are /!" on first load.

Actual Results

  1. Console log shows "_pathChanged has big mood: /" and the DOM shows "Web Components are great!" on first load.

Browsers Affected

  • Chrome
  • Firefox
  • Edge

(Other browsers probably affected, but I don't have them handy.)

Versions

  • lit-element: v2.0.1
  • webcomponents: v2.2.6
@kevinpschaaf
Copy link
Member

kevinpschaaf commented Feb 7, 2019

Confirmed the issue, which is due to <iron-location> synchronously firing events during creation. A simple solution is to await this.updateComplete in the event handler to ensure the property assignment triggers a re-render.

https://stackblitz.com/edit/lit-element-example-wrnwsh?file=index.js

Longer explanation:

If web components being rendered fire events synchronously to rendering, event handlers that set properties based on those events won't trigger a re-render. Although we do now consider elements that fire events when being created or when properties on them are set to be an anti-pattern (we only recommend firing events as a result of UI/asynchronous actions), legacy elements from Polymer like <iron-location> often did exactly that.

LitElement's implementation of update synchronously calls lit-html render, and if the act of "rendering" any DOM causes synchronous events to fire and set properties, those changes won't trigger a re-render due to the contract around update in UpdatingElement (any changes to properties in update do not cause a second update to be scheduled).

The contact for update around this behavior was intentional: overriding update and changing properties before super.update() is a fine pattern for e.g. resetting state based on property changes, and those changes will still be reflected in render since it will be called in the super.update(). However, changes after rendering (i.e. after super.update()) should be instead done in updated so that they cause a re-render.

We will consider the best way to address this, either in the library or via patterns, and in the fullness of time, ensuring a community-wide pattern where elements don't fire events synchronously seems the best way forward for a number of reasons.

@sorvell sorvell self-assigned this Feb 7, 2019
sorvell pushed a commit that referenced this issue Feb 7, 2019
Fixes #549. Changes `UpdatingElement` such that updates can occur after calling `super.update()`. Changes `LitElement` such that update/render can occur as a result of rendering, for example if an event listener is triggered that sets a property.
@zombiezen
Copy link
Author

Makes sense, thank you for the explanation! Yeah, not immediately retriggering rendering makes a lot of sense, because then you could hang the UI thread with infinite rendering.

@ghost ghost self-assigned this Feb 8, 2019
@ghost ghost added the Area: docs label Feb 8, 2019
@ghost
Copy link

ghost commented Feb 8, 2019

Note to self to address this somehow in docs, maybe Events and/or Lifecycle pages.

@ghost ghost removed their assignment Feb 26, 2019
@arthurevans
Copy link
Contributor

Related to: #551.

justinfagnani pushed a commit that referenced this issue Jan 17, 2020
Fixes #549.

Changes `UpdatingElement` such that updates can occur after calling `super.update()`. Changes `LitElement` such that update/render can occur as a result of rendering, for example if an event listener is triggered that sets a property.

Update should be marked ready for additional/new updates when:
1. After the `update` method is called (e.g. after super.update(...))
2. If `shouldUpdate` return false. Returning false indicates that this update turn should be ignored but future updates should be processed.
3. If there is an exception during `update`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants