lit / lit-element

LEGACY REPO. This repository is for maintenance of the legacy LitElement library. The LitElement base class is now part of the Lit library, which is developed in the lit monorepo.
https://lit-element.polymer-project.org
BSD 3-Clause "New" or "Revised" License
4.49k stars 319 forks source link

Incorrect behaviour when using `super.update()` as shown in the documentation. #1085

Closed jandreola closed 4 years ago

jandreola commented 4 years ago

Description

The documentation and many examples shows that super.update() should be placed at the top when overriding the update method. But if we update any observed property after calling super.update() none of the changes will reflect on the next render.

Live Demo

https://webcomponents.dev/edit/gCiSmxIqNaCBLZTOWlJv In the demo you can uncomment the super.update at the end and you will see it working correctly.

Steps to Reproduce

@customElement("test-update")
export class TestUpdate extends LitElement {
  @property({ attribute: "testing" })
  testing = "Old Value";

  update(a) {
    super.update(a);
    this.testing = "New value"; // <-- This should be rendered
  }

  render() {
    return html`<h1>${this.testing}</h1>`;  // <-- Render doesn't show the new value
  }
}

Expected Results

Documentation should clarify that in order to work as expected super.update() must be called after any updates to observed properties

Actual Results

Documentation is not clear / misleading on where super.update should be called

Browsers Affected

Versions

justinfagnani commented 4 years ago

This is caused by bad TypeScript compiler settings in webcomponents.dev which is triggering this TypeScript bug: https://github.com/microsoft/TypeScript/issues/35081

The solution is to turn off useDefineForClassFields, though I'm not sure if that's possible in webcomponents.dev

justinfagnani commented 4 years ago

Hopefully that's fixed in TypeScript 4.1. In the mean time let's track in https://github.com/Polymer/lit-element/issues/855

sorvell commented 4 years ago

Here's a working example: https://webcomponents.dev/edit/ZVhx2rKR4v6RP43I1rdl.

georges-gomes commented 4 years ago

Hi @jandreola

Your live demo uses a index.js so Typescript is not involved (side note: we don't set useDefineForClassFields).

If you want to use Typescript, just rename the file to index.ts. If you just want to use Javascript, it seems that our default Babel options are not working.

we use

[ "proposal-decorators", { "legacy": true } ],
[ "proposal-class-properties", { "loose": true } ]

@gluck found that If you change babel options to:

{
  "plugins": [
    [
      "proposal-decorators",
      {
        "decoratorsBeforeExport": true
      }
    ],
    [
      "proposal-class-properties"
    ]
  ]
}

Then the scenario works 👍

This can be done on webcomponents.dev here:

image

@team polymer, What babel options should be used for JavaScript LitElement with decorators?

jandreola commented 4 years ago

@georges-gomes good catch, I didn't notice that the discussion steered towards TS and the example was plain JS. Thank you.

Since TS and Babel causes issues with some default configs, should we warn people somewhere about it? It doesn't seem like an edge case. I also found that unit tests in Lit do call super.update() after changing observed properties, so it feels like a common issue.

justinfagnani commented 4 years ago

Oh, I'm sorry. I saw decorators and assumed TypeScript. We don't have too many people using decorators with Babel.

@georges-gomes this is the .babelrc we use in our tests: https://github.com/Polymer/lit-element/blob/master/.babelrc It uses the formerly "stage 2" decorators, not legacy decorators.

georges-gomes commented 4 years ago

@justinfagnani ok thanks! We will update on our side accordingly.