-
Notifications
You must be signed in to change notification settings - Fork 199
Beef up docs around using class fields #516
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
Conversation
A live preview of this PR will be available at the URL(s) below. https://pr516-f3307b0---lit-dev-5ftespv5na-uc.a.run.app/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! I just have a few suggestions for the "Avoiding issues" section.
|
||
</div> | ||
**You can use class fields** with reactive properties when transpiling code with Babel or TypeScript and the proper settings are enabled. Both Babel and TypeScript support transpiling class fields into properties set in the constructor rather than defined on the instance. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest removing this paragraph, kinda seems like it's mostly redundant with the next two paragraphs, which are more focused.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed.
@@ -73,25 +73,34 @@ In the future when decorators become a native web platform feature, this may no | |||
|
|||
To use decorators with [TypeScript](https://www.typescriptlang.org/docs/handbook/decorators.html), enable the `experimentalDecorators` compiler option. | |||
|
|||
You should also ensure that the `useDefineForClassFields` setting is `false`. Note, this should only be required when the `target` is set to `es2020` or greater, but it's recommended to explicitly ensure this setting is `false`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it's only for esnext
, not es2020
(microsoft/TypeScript#34787)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
Co-authored-by: Alexander Marks <[email protected]>
Co-authored-by: Alexander Marks <[email protected]>
Co-authored-by: Alexander Marks <[email protected]>
Co-authored-by: Alexander Marks <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, we can also now update the /msg/class-field-shadowing
short link to point to this new section!
['/msg/class-field-shadowing', '/docs/components/properties/#declare'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, a couple of wording suggestions.
``` | ||
|
||
Enabling `emitDecoratorMetadata` is not required and not recommended. | ||
|
||
### Using decorators with Babel { #decorators-babel } | ||
|
||
If you're compiling JavaScript with [Babel](https://babeljs.io/docs/en/), you can enable decorators by adding the following plugins: | ||
If you're compiling JavaScript with [Babel](https://babeljs.io/docs/en/), you can enable decorators by adding the following plugins and assumptions: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using "assumptions" here is super jargony. You'd have to be pretty well-versed in Babel to know that there are config options called "Compiler assumptions," and the average user is likely to infer the generic meaning for assumptions here.
Suggest either just go with "plugins" and assume (heh) that the configuration steps that follow are part of adding the plugins, or say, "plugins and settings"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to "plugins and settings"
Co-authored-by: Arthur Evans <[email protected]>
Fixes #462