Moved repo to strict: true and restructured tsconfig.jsons to use project references #567
Conversation
|
…and added strict: true
| "lint": "eslint ./src --ext .ts", | ||
| "format": "prettier --write 'src/**/*.ts?(x)' && npm run lint -- --fix", | ||
| "typecheck": "tsc --noEmit", | ||
| "typecheck": "tsc -b", |
There was a problem hiding this comment.
Now, this typecheck command checks the entire repo, including the typecheck tests.
There was a problem hiding this comment.
Doesn't this create artifacts, though? Shouldn't it be tsc -b --noEmit?
The whole intention of the typecheck script is to typecheck without generating artifacts.
There was a problem hiding this comment.
No, noEmit has been moved to the config file - this is so that users running tsc without using the command don't accidentally generate artifacts.
| @@ -969,9 +962,9 @@ describe('ResultAsync', () => { | |||
| it('Accepts an error handler as a second argument', async () => { | |||
| const example = ResultAsync.fromThrowable( | |||
| () => Promise.reject('No!'), | |||
| (e) => new Error('Oops: ' + e) | |||
| (e) => new Error('Oops: ' + e), | |||
There was a problem hiding this comment.
Looks like prettier changes in this file.
| "exclude": [], | ||
| "references": [ | ||
| { | ||
| "path": "./tsconfig.tests.json" |
There was a problem hiding this comment.
This is now referenced in the root tsconfig, meaning both can be run simultaneously.
mattpocock
changed the title
| "declaration": true, | ||
| "baseUrl": "./src", | ||
| "lib": [ | ||
| "dom", |
There was a problem hiding this comment.
Removed the DOM types, not needed here.
| "sourceMap": false, | ||
| "noUnusedLocals": true, | ||
| "noUnusedParameters": true, | ||
| "strictNullChecks": true, |
There was a problem hiding this comment.
Slimmed down the tsconfig by flags inferred by other flags.
There was a problem hiding this comment.
I don't think neverthrow should include editor specific files. Please remove.
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| return ((...args: any[]) => { | ||
| try { | ||
| const result = fn(...args) | ||
| return ok(result) | ||
| } catch (e) { | ||
| return err(errorFn ? errorFn(e) : e) | ||
| } | ||
| } | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| }) as any |
There was a problem hiding this comment.
Can you explain the rationale for these as any changes made here and in other files?
There was a problem hiding this comment.
Moving to strict: true revealed some errors that I didn't have time to debug. I suggest we 'as any' them now, and fix them in a followup PR.
There was a problem hiding this comment.
The reason this errors and you have to as any it is because with this call signature you can in theory do something like:
const safeParse = fromThrowable<typeof JSON.parse, number>(JSON.parse);
// ^ (text: string, reviver?: (this: any, key: string, value: any) => any) => Result<any, number>So by allowing providing a specific generic type as E without providing an errorFn, you can get in trouble. You can then end up in a case where you try to pass e to the err in the catch block (in the second/false ternary case), but since the user might have specified a specific E (like number in the example above), you can't just give it the unknown (or any here I suppose)e.
The easiest fix might be to add some overloads to prevent using the function as I did above:
export function fromThrowable<Fn extends (...args: readonly any[]) => any>(
fn: Fn
): (...args: Parameters<Fn>) => Result<ReturnType<Fn>, unknown>;
export function fromThrowable<Fn extends (...args: readonly any[]) => any, E>(
fn: Fn,
errorFn: (e: unknown) => E,
): (...args: Parameters<Fn>) => Result<ReturnType<Fn>, E>;
export function fromThrowable<Fn extends (...args: readonly any[]) => any, E>(
fn: Fn,
errorFn?: (e: unknown) => E,
): (...args: Parameters<Fn>) => Result<ReturnType<Fn>, E | unknown> {
// implementation
}I don't know if it really makes any practical difference here, but I believe it would be more correct to do catch (e: unknown) to have e typed as unknown and not the default any.
This could of course happen in a different PR. And I suppose in theory changing the call signature as I proposed might be considered a breaking change. However, if someone is using the function as I did above they should stop doing as it probably gives you types that don't match the runtime types. So you could consider it a bug maybe 🤷
There was a problem hiding this comment.
I created a separate PR for fixing the issue: #583 However, feel free to close that PR and include the changes here instead if that's better.
| "lint": "eslint ./src --ext .ts", | ||
| "format": "prettier --write 'src/**/*.ts?(x)' && npm run lint -- --fix", | ||
| "typecheck": "tsc --noEmit", | ||
| "typecheck": "tsc -b", |
There was a problem hiding this comment.
Doesn't this create artifacts, though? Shouldn't it be tsc -b --noEmit?
The whole intention of the typecheck script is to typecheck without generating artifacts.
| "declaration": true, | ||
| "composite": true, | ||
| "outDir": "./tests/dist", | ||
| "noEmit": false |
There was a problem hiding this comment.
Shouldn't this be "noEmit": true?
There was a problem hiding this comment.
Using project references means that you do need to generate declaration files. This acts as a kind of cache for TS, making things run a bit faster.
These files are outputted to ./tests/dist, as specified in outDir
No changeset needed, not a user-facing change.