refactor: enable a few TS flags #6852
Conversation
Josh-Cena
added
the
pr: maintenance
|
✔️ [V2] 🔨 Explore the source changes: 9d8b184 🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/62243ef98aeecf0007099ca8 😎 Browse the preview: https://deploy-preview-6852--docusaurus-2.netlify.app |
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-6852--docusaurus-2.netlify.app/ |
|
Size Change: +103 B (0%) Total Size: 791 kB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
What about using this base config?
We'll stay very strict as options get added:
https://www.npmjs.com/package/@tsconfig/strictest
Honestly not fan of noUncheckedIndexedAccess, it's not an option a lot of projects turn on and these ! everywhere only add noise
| @@ -51,14 +51,13 @@ export function createExcerpt(fileString: string): string | undefined { | |||
|
|
|||
| // Skip code block line. | |||
| if (fileLine.trim().startsWith('```')) { | |||
| const codeFence = fileLine.trim().match(/^`+/)![0]!; | |||
There was a problem hiding this comment.
🤔 I have no idea what is going on here and that looks like a risky change. Are tests covering this?
There was a problem hiding this comment.
Yes, we do have tests for this. We also have edge cases for this on our own website, so if our website builds, it's good!
That one has some configurations that we not necessarily agree with / are compatible with. For example,
I think it makes sense, particularly when merging translations so we don't assume that all translation keys are present. I can't remember but maybe I fixed a bug a while ago related to this? It seems we are overall very cautious with index access in general, and remaining places just require slapping a |
I agree but I'd rather disable these specific rules from a base strict config rather than enabling other rules on a less strict config => when a new strictness rule is introduced, it becomes automatically enabled so we don't forget to enable it in the future Ok we can keep I find it painful in a few places like theming/CSS modules, maybe we don't want these ! to end-up in swizzled TS files too, so why not disable it in themes only for example? |
Yup, that's workable. Just unsure if it reduces type-safety elsewhere... Ah, I wish TS is as configurable as a linter... |
Motivation
Stricter compilation. Although 95% of them just require a
!, there are actually a few potentially unsound accesses.Have you read the Contributing Guidelines on pull requests?
Yes