Enable TypeScript strictNullChecks

[silverwind]

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 off with type assertions on e.target.

[lunny]

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

[silverwind]

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.

[delvh]

LGTM apart from the regression

[silverwind]

@wxiaoguang for some reason you comments show up on the wrong lines. Can you correct them?

[wxiaoguang]

@wxiaoguang for some reason you comments show up on the wrong lines.

GitHub's bug:

[silverwind]

FYI comments show correctly in this link: d6dc25a

[wxiaoguang]

Now the remaining problem (not in this PR's scope) is: how to handle the strict types clearly.

In our case, most are caused by querySelector and getAttribute. Actually we clearly know that these values must exist, but they are from HTML rendered by tmpl.

[silverwind]

In our case, most are caused by querySelector and getAttribute. Actually we clearly know that these values must exist, but they are from HTML rendered by tmpl.

Generally I prefer to code defensively and stop the current operation, e.g. if guards.

For cases where one is absolutely sure a element exists, ! can be used, but even then, nothing is guaranteed in the DOM. There could be a browser extension or user customization JS messing with the DOM for example.

[wxiaoguang]

In our case, most are caused by querySelector and getAttribute. Actually we clearly know that these values must exist, but they are from HTML rendered by tmpl.

Generally I prefer to code defensively and stop the current operation, e.g. if guards. For cases where one is absolutely sure a element exists, ! can be used, but even then, nothing is guaranteed in the DOM. There could be a browser extension messing with the DOM for example.

I think some util functions can help:

const el = mustQueryElem(document, '#xxxxx');
const val = mustGetAttr(el, 'data-my-attr');

In the must functions, if target doesn't exist, throw errors. Then we can catch the bugs as early as possible.

[silverwind]

Generally I prefer to use as few such wrappers as possible, and I don't see a major benefit in using such must* wrappers. Wrappers makes it harder to contribute because one needs to familiarize with wrappers first.

[wxiaoguang]

In the must functions, if target doesn't exist, throw errors.

Without this approach, some bugs will be difficult to find. For example: some data- attributes are missing. The ! helps nothing.

[silverwind]

Yes, I noticed typescript does not complain about things like `${el.getAttribute('nonexist'}` which will stringify to 'null', so yes, some checks on this would be good. I think there is a eslint rule available for exactly this problem somewhere.

[silverwind]

Relevant rule is: https://typescript-eslint.io/rules/restrict-template-expressions/ with allowNullish set to false.