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

Support for class fields #3278

Closed
LarsDenBakker opened this issue Jul 4, 2020 · 4 comments
Closed

Support for class fields #3278

LarsDenBakker opened this issue Jul 4, 2020 · 4 comments
Assignees

Comments

@LarsDenBakker
Copy link
Contributor

LarsDenBakker commented Jul 4, 2020

This issue has been reported before with typescript (lit/lit-element#855 and lit/lit-element#878)

With class fields coming to Safari 14, we will start to see more developers using it outside of typescript.

The class field specification changed from using assignment to using defineProperty. This is a problem when using LitElement properties without decorators.

This is a small example: https://jsbin.com/zesedefeqe/1/edit?html,output

Is there any way we can support standard class fields in JS without decorators? Otherwise this will become a common source for confusion for developers.

@sorvell
Copy link
Member

sorvell commented Jul 9, 2020

Unfortunately, it does not appear there's anything good we can do to make this better. The spec went in a direction where this feature is really kind of incomplete until decorators are fully supported. Hopefully, that will happen soon.

To use class fields, you'll need to use our @property decorator. Otherwise, when using static properties to define properties, properties must be initialized in the constructor and now with a class field.

I think the example you linked was incomplete. Here's an updated one: https://jsbin.com/tulowur/edit?html,output.

It is possible to make this work right now via a workaround. We have support for handling properties set early when an element definition is not yet defined; however, this tracking is done in the constructor, before class fields are defined. It's possible to manually call _saveInstanceProperties, say, in connectedCallback to make this work for class fields. We don't want to implement this generally because it's highly likely to introduce a significant performance penalty. The idea is that when you lazy load your definition, the performance penalty of modifying the class is offset by the benefit of loading the definition lazily.

https://jsbin.com/cojivaf/2/edit?html,output

connectedCallback() {
  this._saveInstanceProperties();
  super.connectedCallback();
}

@sorvell
Copy link
Member

sorvell commented Jul 9, 2020

@arthurevans Please update the docs to explicitly state the class fields should not be used in Javascript, explaining the reason and how to initialize properties in the constructor. Thanks!

@43081j
Copy link
Collaborator

43081j commented Aug 31, 2020

So when decorators one day get agreed upon (its taking its time), this will be solved, right? assuming we rewrite all our decorators.

e.g.

export decorator @customElement(name) {
  @register(cls => customElements.define(name, cls))
}
export decorator @property(opts) {
  @initialize((target, name, value) => {
    target[`__${name}`] = value;
  })
  @register((target, name) => {
    Object.defineProperty(target, target.getPropertyDescriptor(name, `__${name}`, opts));
  })
}

but given how many implementations of different decorator proposals are in the wild already, do we even have any idea how long this might take? it sounds like its not any time soon, as always with decorators, so we will just have to recommend against class fields for potentially months?

thats ok if its the case i just want to understand the plan

sticky situation for sure.. since we can't even move property logic into the constructor as it'll still run before the define calls do. it seems if you want to use class fields, you will inevitably have to change your code to call something after your constructor to make this work.

on a side note can we merge all these issues (or close the duplicates, theres 3-4 already) and track this somewhere we can watch?

haoqunjiang referenced this issue in haoqunjiang/vite Jul 14, 2021
Since TypeScript 4.3, `target: "esnext"` indicates that
`useDefineForClassFields: true` as the new default.
See <microsoft/TypeScript#42663>

So I'm explicitly adding this field to the tsconfigs to avoid any
confusions.

Note that `lit-element` projects must use
`useDefineForClassFields: false` because of <https://github.com/lit/lit-element/issues/1030>

Vue projects must use `useDefineForClassFields: true` so as to support
class style `prop` definition in `vue-class-component`:
<vuejs/vue-class-component#465>

Popular React state management library MobX requires it to be `true`:
<https://mobx.js.org/installation.html#use-spec-compliant-transpilation-for-class-properties>

Other frameworks seem to have no particular opinion on this.

So I turned it on in all templates except for the `lit-element` one.
@kevinpschaaf kevinpschaaf transferred this issue from lit/lit-element Sep 8, 2022
@justinfagnani
Copy link
Collaborator

I think we can close this for now. We've documented that you shouldn't use class fields in JavaScript, and ways to get around the problem in TypeScript with "useDefineForClassFields": false or declare. The stage 3 decorators proposal also solves with with the accessor keyword.

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

No branches or pull requests

5 participants