chore: prevent unchecked index access #1612
Conversation
weboko
force-pushed
the
weboko/ts-warning
branch
from
26bf15b
to
9e12340
Compare
size-limit report 📦
|
| const tmp = arr[i] as T; | ||
| arr[i] = arr[j] as T; |
There was a problem hiding this comment.
does it not automatically infer the type? arr is of the type T[] 🤔
There was a problem hiding this comment.
noUncheckedIndexedAccess makes so anything that is accepted by index becomes T | undefined
| const clusterId = parseInt(parts[4] as string); | ||
| const shard = parseInt(parts[5] as string); |
There was a problem hiding this comment.
nit: similar comment as above of not automatically inferring the types
| @@ -3,6 +3,7 @@ | |||
| "compilerOptions": { | |||
| "module": "ESNext", | |||
| "moduleResolution": "Bundler", | |||
| "noEmit": true | |||
| "noEmit": true, | |||
| "noUncheckedIndexedAccess": false, | |||
There was a problem hiding this comment.
why is this false here? 🤔
There was a problem hiding this comment.
tsconfig.dev is used for tests where we don't really need to enforce this rule
There was a problem hiding this comment.
wouldn't it be still good to have tests with no unchecked index access? can see it infact being more useful because it would spot errors before execution level :D
There was a problem hiding this comment.
not sure, let's discuss
I can enable it for tests but in that case many utilities we write would need to be compliant with more eager checks
I am in favor of enabling it for tests too tbh
@adklempner wdyt?
There was a problem hiding this comment.
Sounds beneficial but warrants a separate PR imo
| { | ||
| test: /\.([cm]?ts|tsx)$/, | ||
| loader: "ts-loader", | ||
| options: { configFile: tsConfigFile }, |
There was a problem hiding this comment.
This change makes tests running in with tsconfig.dev.json which makes sense as we do have a lower bar for linting tests.
@waku-org/js-waku-developers
| "test": "NODE_ENV=test npm run test --workspaces --if-present", | ||
| "test:browser": "NODE_ENV=test npm run test:browser --workspaces --if-present", | ||
| "test:node": "NODE_ENV=test npm run test:node --workspaces --if-present", | ||
| "test": "npm run test --workspaces --if-present", |
There was a problem hiding this comment.
Is not needed as each of internal commands should be worrying about supplying ENV flags
fyi @danisharora099
Problem
When access happens by index it is possible to hit a case when no value returned.
With no proper handling of such edge case it's easy to hit a problem in run time and increase amount of problems for consumers.
Examples of such issue is here - #1562
Solution
Enable TypeScript to check for such a case
Notes