Enable TypeScript strictNullChecks
#35843
Conversation
GiteaBot
added
the
lgtm/need 2
silverwind
added
the
type/refactoring
|
Too many |
|
Might be possible to move up the Also, I don't think we should relax any of the |
| const refStyle = window.getComputedStyle(styleElement.value); | ||
| const refAltStyle = window.getComputedStyle(altStyleElement.value); | ||
| const refStyle = window.getComputedStyle(styleElement.value!); | ||
| const refAltStyle = window.getComputedStyle(altStyleElement.value!); |
There was a problem hiding this comment.
I don't think it's right to keep adding ! everywhere.
If you are refactoring, then it should follow the best practice and make the code readable / maintainable -- unless you will do quick following up changes to fix them in short time.
Just like the "jQuery removal", adding a lot ? only makes code difficult to read or maintain. At last I have to clean up them one by one to really finish the refactoring.
For this one, maybe the proper fix is to use useTemplateRef<T>
And a lot of other places also need a proper fix.
There was a problem hiding this comment.
Yes, it is better to try to ensure stuff is non-nullable in first place, I will see what I can do, but some stuff like DOM queries are unfixable without major refactors that adds ifs everywhere and I think I will defer that to later and keep the !.
| if (!window.isSecureContext && elSignInPasskeyBtn) { | ||
| hideElem(elSignInPasskeyBtn); |
There was a problem hiding this comment.
| if (!window.isSecureContext && elSignInPasskeyBtn) { | |
| hideElem(elSignInPasskeyBtn); | |
| if (!window.isSecureContext) { | |
| if(elSignInPasskeyBtn) hideElem(elSignInPasskeyBtn); |
| }); | ||
| } catch (error) { | ||
| console.error(error); | ||
| // @ts-expect-error: TODO |
There was a problem hiding this comment.
I think this was some index signature issue, will check.
GiteaBot
added
lgtm/need 1
5 participants
A big step towards enabling strict mode in Typescript.
There was definitely a good share of potential bugs while refactoring this. When in doubt, I opted to keep the potentially broken behaviour. Notably, the
DOMEventtype is gone, it was broken and we're better of with type assertions one.target.