Skip to content
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.

Sign up for GitHub

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

Jump to bottom

refactor(treewide): Enable strict null checking #9975

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

refactor(treewide): Enable strict null checking #9975

wants to merge 2 commits into from

Commits on Sep 11, 2024

  1. test: Add betterer 

    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
    @Minion3665
    Minion3665 committed Sep 11, 2024
    Configuration menu
    Copy the full SHA
    c999565 View commit details
    Browse the repository at this point in the history

  2. refactor(treewide): Enable strict null checking 

    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
    Minion3665 committed Sep 11, 2024
    Configuration menu
    Copy the full SHA
    24c631b View commit details
    Browse the repository at this point in the history