refactor(treewide): Enable strict null checking #9975
Conversation
| @@ -12,7 +12,8 @@ | |||
| "removeComments": false, | |||
| "preserveConstEnums": false, | |||
| "downlevelIteration": true, | |||
| "watch": false, | |||
There was a problem hiding this comment.
I removed this because it has no effect - we would have to use https://www.typescriptlang.org/docs/handbook/configuring-watch.html#configuring-file-watching-using-a-tsconfigjson if we wanted to configure watch options. Although if this is false (the default) it's doubly-ineffective. It's not strictly related to strictNullChecks
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
CI fails because
I think I need to set up https://github.com/phenomnomnominal/betterer or something at this point - because I don't really want to set the max warnings to 842 🥲 |
Minion3665
force-pushed
the
private/skyler/push-zvmluowslutr
branch
2 times, most recently
from
dd941e2
to
333cf7d
Compare
|
@eszkadev - as the CI failure shouldn't require me changing the typescript again, I've requested a review now (I suspect this is both something that may be annoying to continually rebase and something that will take a long time to look through - despite changing very little about the emitted code). I apologize for the size of the diff and all the extra warnings this causes - I think it's preferable to not having them and unfeasible to fix them all at once - but it's still not great. |
|
I guess the other thing I should mention here is that this was a side-quest on my way to doing #8221 (because I needed to type something as null at one point... and it was leaking an unknown into a place it shouldn't have been which made me realize this option was off) |
Minion3665
force-pushed
the
private/skyler/push-zvmluowslutr
branch
9 times, most recently
from
3223f59
to
256ff65
Compare
| const height = aBBox.height / aActivityParamSet.nSlideHeight; | ||
| const x = (aBBox.x + aBBox.width / 2) / aActivityParamSet.nSlideWidth; | ||
| const y = (aBBox.y + aBBox.height / 2) / aActivityParamSet.nSlideHeight; | ||
| const width = aBBox!.width / aActivityParamSet.nSlideWidth; |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note
| const x = (aBBox.x + aBBox.width / 2) / aActivityParamSet.nSlideWidth; | ||
| const y = (aBBox.y + aBBox.height / 2) / aActivityParamSet.nSlideHeight; | ||
| const width = aBBox!.width / aActivityParamSet.nSlideWidth; | ||
| const height = aBBox!.height / aActivityParamSet.nSlideHeight; |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note
| const y = (aBBox.y + aBBox.height / 2) / aActivityParamSet.nSlideHeight; | ||
| const width = aBBox!.width / aActivityParamSet.nSlideWidth; | ||
| const height = aBBox!.height / aActivityParamSet.nSlideHeight; | ||
| const x = (aBBox!.x + aBBox!.width / 2) / aActivityParamSet.nSlideWidth; |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note
| const width = aBBox!.width / aActivityParamSet.nSlideWidth; | ||
| const height = aBBox!.height / aActivityParamSet.nSlideHeight; | ||
| const x = (aBBox!.x + aBBox!.width / 2) / aActivityParamSet.nSlideWidth; | ||
| const y = (aBBox!.y + aBBox!.height / 2) / aActivityParamSet.nSlideHeight; |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note
Betterer is a snapshot test runner which tries to reduce the number of errors. With, for example, eslint, we need to get to 0 errors for the test to pass. This is prohibitive for large changes (e.g. turning on a new eslint option), as you then have to make a lot of changes in the same pull request. For betterer, you only have to avoid making the test fail *more*, so it's OK to fix errors or leave things alone. --- It runs as a part of check, and outputs (but doesn't block) building. You can also run it manually, in the same way as prettier or eslint $ make betterer And get it to update its snapshots, again in the same way as prettier $ make betterer-write --- I avoided adding the betterer precommit, as running eslint takes a long while and this blocked the commit for longer than I considered acceptable. There's probably a way to make this work with eslint-d and a custom test, but even so it's likely custom, brittle and non-ideal. Signed-off-by: Skyler Grey <[email protected]> Change-Id: I1770a1a8be3f45b45bda0fd2bcd57563add25924
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
Minion3665
force-pushed
the
private/skyler/push-zvmluowslutr
branch
from
256ff65
to
24c631b
Compare
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 xeverywhere. 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 ~2000 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.
Change-Id: I1770a1a8be3f45b45bda0fd2bcd57563add25924
Summary
TODO
Checklist
make prettier-writeand formatted the code.make checkmake runand manually verified that everything looks okay