refactor(ts): strict mode #696
Conversation
| try { | ||
| return Storage.isIdentifierValid(value, true); | ||
| } catch (err) { | ||
| } catch (err: any) { |
There was a problem hiding this comment.
I'm gonna swap these out with the useUnknownInCatchVariables TSConfig option.
Co-authored-by: Kanad Gupta <kanad@readme.io>
erunion
force-pushed
the
ts-strict-mode
branch
from
9a42479 to
bea445b
Compare
| const parameters = {} as { | ||
| body: OptionalKind<ParameterDeclarationStructure>; | ||
| metadata: OptionalKind<ParameterDeclarationStructure>; | ||
| }; |
There was a problem hiding this comment.
This slight reshuffling ended up fixing all of those ts-morph typing issues. When we use parameters.body and parameters.metadata they will be present so marking them as optional here is wrong. I believe I had originally done it this way to satisfy setting parameters to an empty object.
There was a problem hiding this comment.
Gotta love strict mode
|
@kanadgupta I've rebranched branched this off my core refactor and rewrote your commits on top of that. I've also fixed the remaining strict issues in the I'm happy with it but please give it another look. |
| "outDir": "dist/" | ||
| "outDir": "dist/", | ||
| "strict": true, | ||
| "useUnknownInCatchVariables": false |
There was a problem hiding this comment.
Allows us to not have to do catch (err: any) nonsense that adds no value.
There was a problem hiding this comment.
Oh hell yeah I've been running into this a bunch in rdme, gonna steal this. Thanks!
| const security = operation.prepareSecurity(); | ||
| Object.keys(security).forEach((id: SecurityType) => { | ||
| security[id].forEach(scheme => { | ||
| Object.entries(operation.prepareSecurity()).forEach(([, schemes]) => { |
There was a problem hiding this comment.
Rewrote this whole loop because I don't know what I was smoking when I did that object.keys() mess earlier. 🪴
| const opts = { | ||
| ...options, | ||
| }; | ||
| } as APIOptions; |
There was a problem hiding this comment.
Eventhough all of our APIOptions are required for this snippet httpsnippet core marks all client options as optional so doing new Oas(opts.apiDefinition) thought we were trying to pass new Oas(undefined). Forcefully re-casing options as options here fixes that.
| .getImportDeclarations() | ||
| .find(id => id.getText().includes('HTTPMethodRange')) | ||
| .replaceWithText("import type { ConfigOptions, FetchResponse } from '@api/core'"); | ||
| ?.replaceWithText("import type { ConfigOptions, FetchResponse } from '@api/core'"); |
There was a problem hiding this comment.
You originally had these as the following but I think optionally chaining these is a little nicer.
const declaration = sdkSource.getImportDeclarations(...).find(...);
if (declaration) declaration.replaceWithText(...);
Cool with the original solution if you want me to revert it though.
There was a problem hiding this comment.
Yeah I suppose this just returns undefined and doesn't actually touch the original sdkSource variable if it can't find anything right? Works for me!
|
@erunion LGTM, thanks for getting this across the finish line 🫶🏽 feel free to merge it in when you're ready! |
🧰 Changes
Flips TS
strictmode on in all packages.fixes RM-7991