Skip to content

Conversation

@silverwind
Copy link
Member

@silverwind silverwind commented Nov 3, 2025

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 DOMEvent type is gone, it was broken and we're better of with type assertions on e.target.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 3, 2025
@silverwind silverwind changed the title Enable strictNullChecks Enable TypeScript strictNullChecks Nov 3, 2025
@silverwind silverwind added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Nov 3, 2025
@lunny
Copy link
Member

lunny commented Nov 13, 2025

Too many ! in the class variables, looks ugly! could we assigned anything on the constructor to avoid them? Or we can disable strictPropertyInitialization.

@silverwind
Copy link
Member Author

silverwind commented Nov 13, 2025

Might be possible to move up the ! to the constructor, yes. Generally, thought there are many unavoidable ! for anything queried out of the DOM and I don't think there is a way to avoid those.

Also, I don't think we should relax any of the strict options, unless there is a clear benefit.

const refStyle = window.getComputedStyle(styleElement.value);
const refAltStyle = window.getComputedStyle(altStyleElement.value);
const refStyle = window.getComputedStyle(styleElement.value!);
const refAltStyle = window.getComputedStyle(altStyleElement.value!);
Copy link
Contributor

@wxiaoguang wxiaoguang Nov 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@silverwind silverwind Nov 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 !.

Copy link
Member

@delvh delvh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM apart from the regression

Comment on lines +15 to 16
if (!window.isSecureContext && elSignInPasskeyBtn) {
hideElem(elSignInPasskeyBtn);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (!window.isSecureContext && elSignInPasskeyBtn) {
hideElem(elSignInPasskeyBtn);
if (!window.isSecureContext) {
if(elSignInPasskeyBtn) hideElem(elSignInPasskeyBtn);

});
} catch (error) {
console.error(error);
// @ts-expect-error: TODO
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Nov 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm/need 1 This PR needs approval from one additional maintainer to be merged. modifies/frontend type/refactoring Existing code has been cleaned up. There should be no new functionality.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants