TSM-04: convert core entry and shared infra to TypeScript - #561
Conversation
0af98c1 to
de5a155
Compare
58095f6 to
97ce5ac
Compare
9ef2479 to
809c70a
Compare
809c70a to
52b267f
Compare
52b267f to
d3aceac
Compare
d3aceac to
a1c30e4
Compare
Alex Matthews (ralexmatthews)
left a comment
There was a problem hiding this comment.
Nice, mostly looks good. I would just strongly suggest keeping noImplicitAny disabled here.
Would also suggest stronger types on the client options, but that one is nonblocking.
| httpMiddleware?: any; | ||
| requestMiddleware?: any; | ||
| httpClient?: any; |
There was a problem hiding this comment.
Question:
Hmmm... are these just stubs for later, or are these actually anything? For example, httpClient could now be initialized with 2 and it would be valid according to TS.
Ideally we would type these with what we expect. For example, httpClient: typeof fetch or similar.
| "compilerOptions": { | ||
| "allowJs": true, | ||
| "checkJs": false, | ||
| "noImplicitAny": false, |
There was a problem hiding this comment.
Suggestion:
I would strongly argue to keep this enabled. This can very easily allow type errors to get swept under the rug. If you create a new function, this no longer requires that function's parameters to be typed.
const getFooBar = (arg) => arg.foo.bar;
// ...
getFooBar("abc")now compiles fine, even though this will be a runtime error.
Summary
Validation