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

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

Closed
1 of 6 tasks
jandreola opened this issue Oct 8, 2020 · 7 comments
Closed
1 of 6 tasks

Comments

@jandreola
Copy link

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

  • Chrome
  • Firefox
  • Edge
  • Safari 11
  • Safari 10
  • IE 11

Versions

  • lit-element: 2.4.0
  • webcomponents: vX.X.X
@justinfagnani
Copy link
Contributor

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

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

@justinfagnani
Copy link
Contributor

Hopefully that's fixed in TypeScript 4.1. In the mean time let's track in #855

@sorvell
Copy link
Member

sorvell commented Oct 8, 2020

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

@georges-gomes
Copy link

georges-gomes commented Oct 9, 2020

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
Copy link
Author

@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
Copy link
Contributor

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
Copy link

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

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

No branches or pull requests

4 participants