fix: use useDefineForClassFields in ts-config#4224
Conversation
31de974 to
f6c5f12
Compare
Codecov Report
@@ Coverage Diff @@
## master #4224 +/- ##
=======================================
Coverage 98.37% 98.37%
=======================================
Files 202 202
Lines 7260 7260
Branches 2119 2119
=======================================
Hits 7142 7142
Misses 58 58
Partials 60 60
Continue to review full report at Codecov.
|
|
I am a little undecided on this one. While it definitely makes sense, it makes the minified(!) browser bundle 25kB(!) or 6% larger—all of which are defineProperty calls. All of which to catch some edge cases which would not be part of the external API anyway. Also, we do not use setters in class fields. The second issue does apply to us, as evidenced by your changes, but I still do not see the value of those 25kB. As we are only testing the generated output, I wonder if we should rather not use this option deliberately and revisit this topic once we actually ship class fields in our code to end users. Still, your changes in the code itself make sense to me and should work even with this option disabled. |
| export default class ExportShimVariable extends Variable { | ||
| module: Module; | ||
|
|
||
| constructor(module: Module) { |
There was a problem hiding this comment.
One thing we could use more is to use constructor argument declarations for fields, i.e. here
constructor(public readonly module: Module) {...}
We are already using it in some places like Chunk where it saves us quite a bit of mindless copying
lukastaegert
left a comment
There was a problem hiding this comment.
I have disabled the flag for now and would like to merge the branch as it is as I find the changes useful but I am not yet sold 100% on the benefits of the flag. That way, your work is not lost and we can reevaluate later.
This PR contains:
Are tests included?
Breaking Changes?
List any relevant issue numbers:
Description
emits ecmascript standard compliant class fields.
it's the default setting for ES2022 and above, including ESNext.
https://www.typescriptlang.org/tsconfig#useDefineForClassFields
more thorough explainer: https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-7.html#the-usedefineforclassfields-flag-and-the-declare-property-modifier
-> the first commit is expected to fail the tests as some of the classes are not ecmascript compliant.