Enable useDefineForClassFields TS flag
#7563
Conversation
GytisCepk
requested review from
aruniverse,
calebmshafer,
wgoehrig,
a team,
ColinKerr,
christophermlawson and
StefanApfel-Bentley
as code owners
|
@wgoehrig when you get a chance, can you review this PR? |
|
This pull request is now in conflicts. Could you fix it @GytisCepk? 🙏 |
| this.origin = props.origin ? Point3d.fromJSON(props.origin) : undefined; | ||
| this.yaw = props.yaw ? Angle.fromJSON(props.yaw).degrees : undefined; | ||
| this.pitch = props.pitch ? Angle.fromJSON(props.pitch).degrees : undefined; | ||
| this.roll = props.roll ? Angle.fromJSON(props.roll).degrees : undefined; |
There was a problem hiding this comment.
These are nice fixes. Frankly I'm surprised that no one has hit this in all these years... Can you also add unit tests that exercise this so we can avoid regressions as we refactor all these Entity subclasses?
| this.type = props.type; | ||
| this.description = props.description; |
There was a problem hiding this comment.
For fields like these (where the type annotation already matches what the Entity constructor was setting), why not just use the declare keyword instead?
| @@ -477,6 +477,7 @@ export abstract class GeometricElement extends Element { | |||
| super(props, iModel); | |||
| this.category = Id64.fromJSON(props.category); | |||
| this.geom = props.geom; | |||
| this.elementGeometryBuilderParams = props.elementGeometryBuilderParams; | |||
There was a problem hiding this comment.
This class field is interesting... @GytisCepk were there any test or build failures that prompted you to add this?
I did some debugging and elementGeometryBuilderParams definitely is not set by the Entity constructor, and it also doesn't get serialized in GeometricElement.toJSON(). However, the corresponding GeometricElementProps.elementGeometryBuilderParams member is used in a bunch of places...
@pmconne @bbastings is it possible this class field is just broken and has never been used? Should we remove it entirely? If we want to fix it instead, we should probably also make sure it gets included in toJSON()...
TypeScript recommends setting
useDefineForClassFieldstotrue(default value starting ES2022). However, enabling this flag caused several issues. More info: #7474Additionally, noticed that some TypeScript types are incorrectly defined (e.g.
AuxCoordSystem2d.angleis defined asnumber, but in reality it can be anumber,objectorundefined). This PR fixes some of these.