chore: enable strict type checking #80
Conversation
backend/lib/deal-observer.js
Outdated
| * @returns {Promise<Array<Static <typeof ActiveDealDbEntry>>>} | ||
| */ | ||
| export async function loadDeals (pgPool, query, args = []) { | ||
| const result = (await pgPool.query(query, args)).rows.map(deal => { | ||
| const result = (await pgPool.query(query, args)).rows.map((/** @type {any} */ deal) => { |
There was a problem hiding this comment.
The deal is validated shortly after this call using typebox. This means that it is safe to use any type.
There was a problem hiding this comment.
@bajtos what do you think about introducing a new type, called ToBeParsed or similar, which equals any? This way we don't have any any, and can forbid it, but at the same time, signal that this type needs to be parsed, and will be parsed.
There was a problem hiding this comment.
Let's use Record<string, unknown>. I wish node-postgres was using that type for result.rows out of the box. (I'll handle this myself and push a new commit.)
There was a problem hiding this comment.
I am introducing UnknownRow type that we can share. I am happy to discuss a better name for this type.
There was a problem hiding this comment.
Ah, you can do pgPool.query<UnknownRow>(...) in TypeScript, but that's unfortunately not possible with JSDoc-style typings :(
There was a problem hiding this comment.
This conversation is resolved as far as I am concerned. I propose to start a new thread if you disagree with some of the decisions I made in 7fab2e9
There was a problem hiding this comment.
This conversation is resolved as far as I am concerned. I propose to start a new thread if you disagree with some of the decisions I made in 7fab2e9
After merging with main, we now can use SubmittableDeals
unkown types.
There was a problem hiding this comment.
This conversation is resolved as far as I am concerned. I propose to start a new thread if you disagree with some of the decisions I made in 7fab2e9
After merging with main, we now can use
SubmittableDeals, which I believe is a better alternative to
unkowntypes.
I agree it's a better alternative than unknown 👍🏻
There was a problem hiding this comment.
@bajtos can we disallow any in typescript settings?
There was a problem hiding this comment.
@bajtos can we disallow
anyin typescript settings?
Not as far as I know.
There is a typescript-eslint rule no-explicit-any, but IIRC, typescript-eslint does not work with JSDoc-in-JS-files, plus we are using standard.js.
I am going to mention this topic here:
NikolasHaimerl
requested review from
juliangruber and
bajtos
and removed request for
bajtos
…ub.com/filecoin-station/deal-observer into nhaimerl-activate-strict-type-checking
@NikolasHaimerl if you reword this to |
|
I suggest to let TS Wiz @bajtos handle this review 🙏 |
|
Great work, @NikolasHaimerl! |
backend/bin/deal-observer-backend.js
Outdated
| @@ -13,7 +13,7 @@ import { findAndSubmitUnsubmittedDeals, submitDealsToSparkApi } from '../lib/spa | |||
| import { getDealPayloadCid } from '../lib/piece-indexer-service.js' | |||
|
|
|||
| /** @import {Queryable} from '@filecoin-station/deal-observer-db' */ | |||
| /** @import {MakeRpcRequest} from '../lib/typings.js' */ | |||
| /** @import {MakeRpcRequest, GetDealPayloadCid} from '../lib/typings.js' */ | |||
There was a problem hiding this comment.
I think you forgot to push the file '../lib/typings.js', no?
There was a problem hiding this comment.
Yes, sorry for that!
I added my version of the typings file in 4cc2e50, but now I need to fix tsc errors that I did not expect.
|
@bajtos Thanks for the review! After merging this branch and updating it with
Let me know if you have any thoughts! 🚀 |
Signed-off-by: Miroslav Bajtoš <[email protected]>
Signed-off-by: Miroslav Bajtoš <[email protected]>
Signed-off-by: Miroslav Bajtoš <[email protected]>
There was a problem hiding this comment.
I made few more tweaks, the new version looks great now.
@juliangruber would you like to take another look as well?
Co-authored-by: Miroslav Bajtoš <[email protected]>
Co-authored-by: Julian Gruber <[email protected]>
Co-authored-by: Julian Gruber <[email protected]>
This PR proposes the following changes:
Closes #51 .
The proposed solution enables strict type checking for the entire repository.
Wherever possible explicit types are used. In some cases the any time was used and I commented on my reasoning for doing so in this PR.