search

aurelia / template-lint

Sanity check of Aurelia-flavor template HTML
Apache License 2.0
56 stars 17 forks source link

Use TypeScript compiler option `strictNullChecks: true` #132

Closed atsu85 closed 7 years ago

atsu85 commented 7 years ago

see https://github.com/MeirionHughes/aurelia-template-lint/commit/74077908f306f643739af23163d117884d2654ca#commitcomment-19354102

atsu85 commented 7 years ago

the PR is ready to be merged (to feature/rework branch)

MeirionHughes commented 7 years ago

I'm hesitant to enable this. Firstly, run-time null-checks in exported classes are still required because the lib can be (generally) consumed in javascript. Secondly, any typed library you use that doesn't use strictNullChecks could trip you up if they return nulls at run-time for types that are missing the | null. Seems like false-sense of security that comes with the cost of bloating code to mark nullables values (which in runtime are all nullable anyway).

atsu85 commented 7 years ago

Firstly, run-time null-checks in exported classes are still required because the lib can be (generally) consumed in javascript.

Ok, I dropped the last commit, that removed the null checks, so this shouldn't be an issue any more.

Secondly, any typed library you use that doesn't use strictNullChecks could trip you up if they return nulls at run-time for types that are missing the | null.

I don't think it will become a problem. Or could You bring an example where this is the case?

Seems like false-sense of security that comes with the cost of bloating code to mark nullables values.

It seems to me, that currently there isn't much bloating code because of enabling strictNullChecks. Actually I think this PR has made the code more readable - for example methods that could return null.

which in runtime are all nullable anyway

Yes, when consumed directly by thrid party lib (such as Aurelia plugin for some IDE), the nulls could sneak into the code anyway, but i guess these NPE's will manifest themselves quite quickly during plugin development time, and probably we shouldn't worry about it too much. Also this change won't save neither us (nor potential IDE plugin developers) from writing tests.

I'm hesitant to enable this

I was also a bit hesitating if I should enable this flag, but enabling it went easier than i thought and I'm quite pleased with the result. So if it was my decision, I'd merge it (now that I had reverted the last commit, that initially removed some null checks)

MeirionHughes commented 7 years ago

I'll give it a try. Don't like the idea of having to explicitly define a nullable though. Checking for nulls (at the export point) is just a fact of life and all reference types are nullable. :D

MeirionHughes commented 7 years ago

Okay, this is fine. I think I was off put by all the diff changes of your commit. In practice, its not really a hindrance.

atsu85 commented 7 years ago

I'm glad You came to the same conclusion :)