chore: fix remaining strictNullChecks errors; enforce in tsconfig #758
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously these two keys were inconsistently treated as being nullable, undefined, or required. Make them optional (undefined) everywhere, which seems to be author's intent. Also add a simple test to exercise the malformed handling, since that's now explicitly possible. Signed-off-by: Dorian Johnson <[email protected]>
The convention seems to be that statusCode is null when isLoading is true, and assignments are already happening that way, so just update the typings to reflect that. Signed-off-by: Dorian Johnson <[email protected]>
Signed-off-by: Dorian Johnson <[email protected]>
Signed-off-by: Dorian Johnson <[email protected]>
Signed-off-by: Dorian Johnson <[email protected]>
First, getFilterConfigByResource is expected to sometimes return undefined, so mark the signature as such. This fixes a test which mocked out an undefined return value. TBH I'm not sure how the original implmentation passed the type checker, I think there's an `any` in the chain that defeated it. Second, in the call to `connect`, don't specify null for optional props, it confuses the typechecker. Instead just omit them. Signed-off-by: Dorian Johnson <[email protected]>
Build the return via map, not via forEach / push, removes LOC and obviates needing to explictly type it. Also add the any type to the call signature. Previously this was implied due to the opaque type of the return array builder. But as the nearby TODO indicates, the callsites freak out when they learn the return type is `Promise<AxiosResponse<any> | undefined>>[]`, and I don't want to dig deeper into that. Signed-off-by: Dorian Johnson <[email protected]>
Also introduce an annoying workaround to achieve the null action prop Signed-off-by: Dorian Johnson <[email protected]>
…ling test Signed-off-by: Dorian Johnson <[email protected]>
These types are pretty unhelpful, in that there are a bunch of keys that aren't actually used for certain search types, and are thus ignored in most practices. This one is no different than the others, so just specify a random default. I'm not 100% certain in this change, but I can't find anywhere that branches on searchType being null, so I think it's safe. Signed-off-by: Dorian Johnson <[email protected]>
These tests dig deep into the config tree and pull out specific combinations of keys from dicts the author "knows" exist. That's fine, but it does not please the type checker. Instead, blackhole the reference into <any> and add a runtime defininition check, that way if the config structure changes it'll be flagged as a test failure. Signed-off-by: Dorian Johnson <[email protected]>
Signed-off-by: Dorian Johnson <[email protected]>
dorianj
requested review from
allisonsuarez,
dikshathakur3119,
feng-tao,
Golodhros and
ttannis
as code owners
|
Great @Golodhros, can you merge please? Thanks for the review |
Yeah, I will merge tomorrow, I want to give some time to others to check it out just in case! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary of Changes
Fix all remaining strictNullChecks violations. Remove it from
bettererand add totsconfig.json. Once this lands, I'll @channel in front-end that their PRs must be rebased, else there's possibility of merge violations on master.There's still mostly easy fixes (in particular, 3 or 4 things missed in earlier PRs), but also some trickier ones. I've given more details about the trickier changes in the commit messages.
There should be no functional changes, and I've tried to do these in as low-risk a way as possible. Rather than get the "perfect" types in place, I've generally opted to try to just document what's in place. A class of this is places where some properties "should" be optional but currently are mandatory, and therefore require a lot of placeholder objects get passed down (a specific example of this is having to pass down a
ResourceType.tableinside aSearchType.CLEAR_TERMobject)But overall I feel like this should be fairly safe, but would appreciate a somewhat close eye.
Tests
I added one test for some added parsing, but mostly this didn't create new behaviors or branches, so no other new tests.
Documentation
n/a
CheckList
Make sure you have checked all steps below to ensure a timely review.