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

[doc] Add accessor solution option for solving the reactive updates class field error #1243

Closed
AndrewJakubowicz opened this issue Oct 27, 2023 · 5 comments · Fixed by #1244
Closed
Assignees

Comments

@AndrewJakubowicz
Copy link
Contributor

AndrewJakubowicz commented Oct 27, 2023

When using TypeScript, if useDefineForClassFields is true (Note default is true if target is ES2022 or higher, including ESNext; false otherwise)

Then setting class fields will throw an error.

E.g.

@property() greeting = 'welcome';

Throws:

The following properties on element my-element will not trigger updates as expected because they are set using class fields: docsHint, count. Native class fields and some compiled output will overwrite accessors used for detecting changes. See https://lit.dev/msg/class-field-shadowing for more information

Issue

The error link: https://lit.dev/msg/class-field-shadowing doesn't document that a possible solution is to use the accessor keyword (which is forward compatible with standard decorators).

This is a valid solution in Lit 3 with hybrid decorators.

Proposal

It should be documented that the error can be solved with

@property() accessor greeting = 'welcome';

This code works with all combinations of experimentalDecorators and useDefineForClassFields and is forward compatible with standard decorators.

@AndrewJakubowicz AndrewJakubowicz changed the title [doc] [doc] Add accessor solution option for solving the reactive updates class field error Oct 27, 2023
@augustjk
Copy link
Member

I think we should also remove the section about compiling with Babel. https://lit.dev/docs/components/properties/#avoiding-issues-with-class-fields:~:text=When%20compiling%20JavaScript%20with%20Babel
Recommending non-standard JS transform purely for allowing class fields seems like a bad idea. Babel users wanting to use class fields for initializing should be using standard decorators with accessor which is covered in the other page that is linked there.

While we're at it, I also would like to see the earlier section on JavaScript clarified a bit (https://lit.dev/docs/components/properties/#avoiding-issues-with-class-fields:~:text=In%20JavaScript%20you%20must%20not%20use%20class%20fields).

We should show what the wrong syntax is. Just saying "must not use class fields", and showing the constructor code is unclear to me.

Proposal:

class MyElement extends LitElement {
  static properties = {foo: {type: String}}
  foo = 'Default'; // ❌ this will make `foo` not reactive
}
class MyElement extends LitElement {
  static properties = {foo: {type: String}}
  constructor() {
    super();
    this.foo = 'Default'; // ✅ this is good
  }
}

@AndrewJakubowicz
Copy link
Contributor Author

AndrewJakubowicz commented Oct 27, 2023

additionally, the following is also valid:

class MyElement extends LitElement {
  static properties = {foo: {type: String}}
  accessor foo = 'Default'; // ✅ this is good
}

Thoughts about including?

@augustjk
Copy link
Member

Oh, my god, it does work. But only with Babel though so I don't think we should include this for pure JS solution. And.. doing accessor without a decorator just seems wrong. To me, their only purpose is to give decorators access to accessors.

@AndrewJakubowicz
Copy link
Contributor Author

AndrewJakubowicz commented Oct 27, 2023

But only with Babel

It also works in TypeScript if you're avoiding using decorators and have useDefineForClassFields: true. 😛

@augustjk
Copy link
Member

Relevant potential change suggested here too:
#1218 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants