-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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.
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
feat(frontend):add guide for strict typescript settings #12506
feat(frontend):add guide for strict typescript settings #12506
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
2 Skipped Deployments
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great writeup @TkDodo, really well written. Reading this, it feels like we should have a "working with typescript" umbrella item where we add content like this. The reason being that there are a number of other things we should document, for example how to declare component types, do we expect people to type fn return types etc... It's all out of scope here, but something for us to think about going forward
|
||
### Recommended approaches | ||
|
||
- Sometimes, it's enough to change `any` to `unknown` (given that enough type narrowing happens inside the function). `unknown` is totally safe to use, and if you have no errors, this is a good way forward. The above example is not one of them though. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be worth a quick explanation of the difference between any
and unknown
in TypeScript (or a link to an explanation); also, can we explain why the above example isn't a good candidate for unknown
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did that here: bcd6b6d
also renamed e
to event
because I had to use it again in a sentence and it felt better.
### Recommended approaches | ||
|
||
- Sometimes, it's enough to change `any` to `unknown` (given that enough type narrowing happens inside the function). `unknown` is totally safe to use, and if you have no errors, this is a good way forward. The above example is not one of them though. | ||
- Look at the usages of the function and potentially inline it into one of those usages while removing the manual `:any`. You can then check via type inference what the type of e would be in that usage. You can then give it an explicit type, in this case: `React.MouseEvent<HTMLButtonElement>` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'type of e' seems like a typo here, what should 'e' be?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from the example above, e
is the event that is passed to the event handler, which is currently typed as any
:
const handleClick = (e: any) => {
I’ve added a code block around the variable to better distinguish this:
alternatively, I could also rename it to event
if you prefer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Thanks for this big add!
Co-authored-by: Alex Krawiec <alex.krawiec@sentry.io>
DESCRIBE YOUR PR
This PR adds a new guide for working with strict compiler settings in the frontend section, as we’ve recently enabled
noImplicitAny
andnoUncheckedIndexedAccess
intsconfig.json
.The goal of this guide is to give developers an overview of the most common issues they might now face, with examples how to fix and avoid them.
IS YOUR CHANGE URGENT?
Help us prioritize incoming PRs by letting us know when the change needs to go live.
SLA
Thanks in advance for your help!
PRE-MERGE CHECKLIST
Make sure you've checked the following before merging your changes:
EXTRA RESOURCES