Skip to content

Commit

Permalink
refactor(treewide): Enable strict null checking
Browse files Browse the repository at this point in the history
Typescript defaults to treating null/undefined in a way that can leak
them throughout code, and will also cause type issues (unexpected
undefineds/anys) if you use them explicitly

To avoid an unreviewable mess I have either widened types to allow null
(where we own the type) or added non-null assertions (where we do not).
This is preferable to not discerning nulls at all, and while it still
creates a large (by LOC) change, it's hopefully not large (by variation
of things) - and should transpile identically to JavaScript.

There is one point where the code does not transpile identically to
JavaScript. This is because it directly set something that we don't own
which shouldn't be undefined to undefined. This is clearly incorrect,
and I've replaced it with setting to an empty string instead.

If it seems like this is bad practice, that's because it is. You
shouldn't add non-null assertions all over the place because it makes
typescript lie to you. It's of similar badness to adding `as x`
everywhere. Unfortunately, worse practice would be mandating that
typescript lie to you about nulls and undefineds everywhere, as our
tsconfig has been doing. It's far better to limit the lie to these ~850
places where I've had to explicitly tell typescript "the world isn't
actually like this - that's OK, carry on anyway".

If you're worried about blames from this change - first: I believe it's
worth it, second: this change should only affect single lines, so using
--ignore-rev should work very well on it

In the future, probably we should remove some of these assertions,
although some changes we'll have to make to remove them are likely
either tedious or dangerous.

Signed-off-by: Skyler Grey <[email protected]>
Change-Id: I1770a1a8be3f45b45bda0fd2bcd57563add25924
  • Loading branch information
Minion3665 committed Sep 10, 2024
1 parent a386958 commit 3223f59
Show file tree
Hide file tree
Showing 138 changed files with 19,240 additions and 1,916 deletions.
4 changes: 4 additions & 0 deletions browser/.betterer.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@ const eslintIgnorePath = resolve(__dirname, ".eslintignore");
const eslintTypescriptIncludes = async () => (await globby).globbySync(["**/*.ts"], { ignoreFiles: eslintIgnorePath });

module.exports = {
'avoid non-null assertions': async () =>
eslintBetter({
'@typescript-eslint/no-non-null-assertion': 'error',
}).include(await eslintTypescriptIncludes()),
'avoid expressions which have no effect': async () =>
eslintBetter({
'@typescript-eslint/no-unused-expressions': 'error',
Expand Down
1 change: 1 addition & 0 deletions browser/.eslintrc
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@
"no-constant-condition": "off",
"@typescript-eslint/triple-slash-reference": "off",

"@typescript-eslint/no-non-null-assertion": "off", // improving in .betterer.ts
"@typescript-eslint/no-unused-expressions": "off" // improving in .betterer.ts
}
}
Expand Down
23 changes: 13 additions & 10 deletions browser/src/app/GraphicSelectionMiddleware.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,10 @@
*/

class GraphicSelection {
public static rectangle: cool.SimpleRectangle = null;
public static rectangle: cool.SimpleRectangle | null = null;
public static extraInfo: any = null;
public static selectionAngle: number = 0;
public static handlesSection: ShapeHandlesSection = null;
public static handlesSection: ShapeHandlesSection | null = null;

public static hasActiveSelection() {
return this.rectangle !== null;
Expand Down Expand Up @@ -43,8 +43,8 @@ class GraphicSelection {
var videoDesc = JSON.parse(textMsg);

if (this.hasActiveSelection()) {
videoDesc.width = this.rectangle.cWidth;
videoDesc.height = this.rectangle.cHeight;
videoDesc.width = this.rectangle!.cWidth;
videoDesc.height = this.rectangle!.cHeight;
}

// proxy cannot identify RouteToken if it is encoded
Expand Down Expand Up @@ -82,8 +82,8 @@ class GraphicSelection {
}

static renderDarkOverlay() {
var topLeft = new L.Point(this.rectangle.pX1, this.rectangle.pY1);
var bottomRight = new L.Point(this.rectangle.pX2, this.rectangle.pY2);
var topLeft = new L.Point(this.rectangle!.pX1, this.rectangle!.pY1);
var bottomRight = new L.Point(this.rectangle!.pX2, this.rectangle!.pY2);

if (app.map._docLayer.isCalcRTL()) {
// Dark overlays (like any other overlay) need regular document coordinates.
Expand Down Expand Up @@ -138,7 +138,7 @@ class GraphicSelection {
);

if (hasGridOffset)
this.rectangle.moveBy([
this.rectangle!.moveBy([
app.map._docLayer._shapeGridOffset.x,
app.map._docLayer._shapeGridOffset.y,
]);
Expand Down Expand Up @@ -177,10 +177,13 @@ class GraphicSelection {
app.sectionContainer.addSection(this.handlesSection);
}

this.handlesSection.setPosition(this.rectangle.pX1, this.rectangle.pY1);
this.handlesSection!.setPosition(
this.rectangle!.pX1,
this.rectangle!.pY1,
);
extraInfo.hasTableSelection = app.map._docLayer.hasTableSelection(); // scaleSouthAndEastOnly
this.handlesSection.refreshInfo(this.extraInfo);
this.handlesSection.setShowSection(true);
this.handlesSection!.refreshInfo(this.extraInfo);
this.handlesSection!.setShowSection(true);
app.sectionContainer.requestReDraw();
} else if (
this.handlesSection &&
Expand Down
Loading

0 comments on commit 3223f59

Please sign in to comment.