refactor: retroactively eslint other cron files#1993
refactor: retroactively eslint other cron files#1993gobengo wants to merge 112 commits intonftstorage:mainfrom
Conversation
…ts log of retrieval duration
…ith sample value from secrets vault
gobengo
changed the title
packages/cron/src/jobs/dagcargo.js
Outdated
| after.toISOString(), | ||
| ]) | ||
| const total = countRes.rows[0].count | ||
| assert.ok(countRes.rows[0]) |
There was a problem hiding this comment.
now that node.js assert.ok is typed as a tsc type guard, this is how i chose to get type safety.
It's at risk of throwing more AssertionError errors, but at least that's a fail-fast well-before any more mysterious runtime errors we have to track down by trolling through logs.
@alanshaw lmk if you'd rather I just use a type assertion (which may be incorrect at runtime) rather than the asserts.
There was a problem hiding this comment.
We have not typically used assert outside of tests. It seems to add a lot of noise to the code base and (at least for me) is making it quite difficult to read the actual code. It looks like you might end up asserting on types multiple times in multiple places and I think it would be more DRY to just use a @typedef or @type once. There's also probably some performance overhead we're suffering by continually type checking everything.
It definitely makes sense to use assert to validate types for inputs (such as in a REST endpoint, or user input from a CLI) at runtime (and likely in some other situations) but I wouldn't use it to obtain type safety like this in your editor or for the typescript compiler.
There was a problem hiding this comment.
I will update to use less runtime assert and instead use a type assertion via js comment.
|
I have to admit I don't really understand why all the #1945 commits appear in here. I think it's because I didn't rebase, but merged. But the rebase seemed like it would take forever and be bug prone? If it's important to have a cleaner commit log on this, I could use some advice on how to do that. (IMO it isn't too important?) |
packages/cron/src/jobs/dagcargo.js
Outdated
| after.toISOString(), | ||
| ]) | ||
| const total = countRes.rows[0].count | ||
| assert.ok(countRes.rows[0]) |
There was a problem hiding this comment.
We have not typically used assert outside of tests. It seems to add a lot of noise to the code base and (at least for me) is making it quite difficult to read the actual code. It looks like you might end up asserting on types multiple times in multiple places and I think it would be more DRY to just use a @typedef or @type once. There's also probably some performance overhead we're suffering by continually type checking everything.
It definitely makes sense to use assert to validate types for inputs (such as in a REST endpoint, or user input from a CLI) at runtime (and likely in some other situations) but I wouldn't use it to obtain type safety like this in your editor or for the typescript compiler.
|
All the places where the new lint rules were unhappy due to unsafety around |
| body: JSON.stringify({ hashToPin: cid, ...(options || {}) }), | ||
| }) | ||
| ) | ||
| const pin = /** @type {PinataPin} */ (json) |
There was a problem hiding this comment.
wow, do we need the extra json const here to please the types?
8 participants
Motivation: