Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Moved repo to strict: true and restructured tsconfig.jsons to use project references #567

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

Moved repo to strict: true and restructured tsconfig.jsons to use project references #567

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9a30242
Upgraded TS and moved to strict: true
mattpocock Aug 20, 2024
37ef88b
Moved the tsconfig.test.json to be a reference of the root tsconfig, …
mattpocock Aug 20, 2024
6771c47
Fixed CI by downgrading TS again
mattpocock Aug 20, 2024
276b3b6
Reverted some tsconfig.json changes
mattpocock Aug 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .vscode/settings.json
View file
Open in desktop
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think neverthrow should include editor specific files. Please remove.

Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"typescript.tsdk": "node_modules/typescript/lib"
}
21 changes: 11 additions & 10 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 3 additions & 4 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,10 @@
"dist"
],
"scripts": {
"test": "jest && npm run test-types",
"test-types": "tsc --noEmit -p ./tests/tsconfig.tests.json",
"test": "jest",
"lint": "eslint ./src --ext .ts",
"format": "prettier --write 'src/**/*.ts?(x)' && npm run lint -- --fix",
"typecheck": "tsc --noEmit",
"typecheck": "tsc -b",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now, this typecheck command checks the entire repo, including the typecheck tests.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"clean": "rm -rf ./dist ./tmp",
"build": "npm run clean && rollup --config && mv tmp/*.js dist",
"prepack": "npm run build",
Expand Down Expand Up @@ -52,7 +51,7 @@
"ts-jest": "27.1.5",
"ts-toolbelt": "9.6.0",
"tslib": "^2.4.0",
"typescript": "4.7.2"
"typescript": "5.0.2"
},
"keywords": [
"typescript",
Expand Down
3 changes: 2 additions & 1 deletion src/result-async.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,8 @@ export class ResultAsync<T, E> implements PromiseLike<Result<T, E>> {
return new Err(errorFn ? errorFn(error) : error)
}
})(),
)
// eslint-disable-next-line @typescript-eslint/no-explicit-any
) as any
}
}

Expand Down
6 changes: 4 additions & 2 deletions src/result.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,14 +23,16 @@ export namespace Result {
fn: Fn,
errorFn?: (e: unknown) => E,
): (...args: Parameters<Fn>) => Result<ReturnType<Fn>, E> {
return (...args) => {
// 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
Comment on lines +26 to +35
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain the rationale for these as any changes made here and in other files?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@boyeln boyeln Sep 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🤷

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

export function combine<
Expand Down
107 changes: 50 additions & 57 deletions tests/index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -333,15 +333,15 @@ describe('Result.fromThrowable', () => {

// Added for issue #300 -- the test here is not so much that expectations are met as that the test compiles.
it('Accepts an inner function which takes arguments', () => {
const hello = (fname: string): string => `hello, ${fname}`;
const safeHello = Result.fromThrowable(hello);
const hello = (fname: string): string => `hello, ${fname}`
const safeHello = Result.fromThrowable(hello)

const result = hello('Dikembe');
const safeResult = safeHello('Dikembe');
const result = hello('Dikembe')
const safeResult = safeHello('Dikembe')

expect(safeResult).toBeInstanceOf(Ok);
expect(result).toEqual(safeResult._unsafeUnwrap());
});
expect(safeResult).toBeInstanceOf(Ok)
expect(result).toEqual(safeResult._unsafeUnwrap())
})

it('Creates a function that returns an err when the inner function throws', () => {
const thrower = (): string => {
Expand Down Expand Up @@ -376,7 +376,7 @@ describe('Result.fromThrowable', () => {
})

it('has a top level export', () => {
expect(fromThrowable).toBe(Result.fromThrowable)
expect(fromThrowable).toBe(Result.fromThrowable)
})
})

Expand Down Expand Up @@ -407,37 +407,34 @@ describe('Utils', () => {
})

it('Combines heterogeneous lists', () => {
type HeterogenousList = [ Result<string, string>, Result<number, number>, Result<boolean, boolean> ]

const heterogenousList: HeterogenousList = [
ok('Yooooo'),
ok(123),
ok(true),
type HeterogenousList = [
Result<string, string>,
Result<number, number>,
Result<boolean, boolean>,
]

type ExpecteResult = Result<[ string, number, boolean ], string | number | boolean>
const heterogenousList: HeterogenousList = [ok('Yooooo'), ok(123), ok(true)]

type ExpecteResult = Result<[string, number, boolean], string | number | boolean>

const result: ExpecteResult = Result.combine(heterogenousList)

expect(result._unsafeUnwrap()).toEqual(['Yooooo', 123, true])
})

it('Does not destructure / concatenate arrays', () => {
type HomogenousList = [
Result<string[], boolean>,
Result<number[], string>,
]
type HomogenousList = [Result<string[], boolean>, Result<number[], string>]

const homogenousList: HomogenousList = [
ok(['hello', 'world']),
ok([1, 2, 3])
]
const homogenousList: HomogenousList = [ok(['hello', 'world']), ok([1, 2, 3])]

type ExpectedResult = Result<[ string[], number[] ], boolean | string>
type ExpectedResult = Result<[string[], number[]], boolean | string>

const result: ExpectedResult = Result.combine(homogenousList)

expect(result._unsafeUnwrap()).toEqual([ [ 'hello', 'world' ], [ 1, 2, 3 ]])
expect(result._unsafeUnwrap()).toEqual([
['hello', 'world'],
[1, 2, 3],
])
})
})

Expand All @@ -446,7 +443,7 @@ describe('Utils', () => {
const asyncResultList = [okAsync(123), okAsync(456), okAsync(789)]

const resultAsync: ResultAsync<number[], never[]> = ResultAsync.combine(asyncResultList)

expect(resultAsync).toBeInstanceOf(ResultAsync)

const result = await ResultAsync.combine(asyncResultList)
Expand Down Expand Up @@ -481,14 +478,14 @@ describe('Utils', () => {
okAsync('Yooooo'),
okAsync(123),
okAsync(true),
okAsync([ 1, 2, 3]),
okAsync([1, 2, 3]),
]

type ExpecteResult = Result<[ string, number, boolean, number[] ], string | number | boolean>
type ExpecteResult = Result<[string, number, boolean, number[]], string | number | boolean>

const result: ExpecteResult = await ResultAsync.combine(heterogenousList)

expect(result._unsafeUnwrap()).toEqual(['Yooooo', 123, true, [ 1, 2, 3 ]])
expect(result._unsafeUnwrap()).toEqual(['Yooooo', 123, true, [1, 2, 3]])
})
})
})
Expand Down Expand Up @@ -518,37 +515,34 @@ describe('Utils', () => {
})

it('Combines heterogeneous lists', () => {
type HeterogenousList = [ Result<string, string>, Result<number, number>, Result<boolean, boolean> ]

const heterogenousList: HeterogenousList = [
ok('Yooooo'),
ok(123),
ok(true),
type HeterogenousList = [
Result<string, string>,
Result<number, number>,
Result<boolean, boolean>,
]

type ExpecteResult = Result<[ string, number, boolean ], (string | number | boolean)[]>
const heterogenousList: HeterogenousList = [ok('Yooooo'), ok(123), ok(true)]

type ExpecteResult = Result<[string, number, boolean], (string | number | boolean)[]>

const result: ExpecteResult = Result.combineWithAllErrors(heterogenousList)

expect(result._unsafeUnwrap()).toEqual(['Yooooo', 123, true])
})

it('Does not destructure / concatenate arrays', () => {
type HomogenousList = [
Result<string[], boolean>,
Result<number[], string>,
]
type HomogenousList = [Result<string[], boolean>, Result<number[], string>]

const homogenousList: HomogenousList = [
ok(['hello', 'world']),
ok([1, 2, 3])
]
const homogenousList: HomogenousList = [ok(['hello', 'world']), ok([1, 2, 3])]

type ExpectedResult = Result<[ string[], number[] ], (boolean | string)[]>
type ExpectedResult = Result<[string[], number[]], (boolean | string)[]>

const result: ExpectedResult = Result.combineWithAllErrors(homogenousList)

expect(result._unsafeUnwrap()).toEqual([ [ 'hello', 'world' ], [ 1, 2, 3 ]])
expect(result._unsafeUnwrap()).toEqual([
['hello', 'world'],
[1, 2, 3],
])
})
})
describe('`ResultAsync.combineWithAllErrors`', () => {
Expand Down Expand Up @@ -576,15 +570,15 @@ describe('Utils', () => {
})

it('Combines heterogeneous lists', async () => {
type HeterogenousList = [ ResultAsync<string, string>, ResultAsync<number, number>, ResultAsync<boolean, boolean> ]

const heterogenousList: HeterogenousList = [
okAsync('Yooooo'),
okAsync(123),
okAsync(true),
type HeterogenousList = [
ResultAsync<string, string>,
ResultAsync<number, number>,
ResultAsync<boolean, boolean>,
]

type ExpecteResult = Result<[ string, number, boolean ], [string, number, boolean]>
const heterogenousList: HeterogenousList = [okAsync('Yooooo'), okAsync(123), okAsync(true)]

type ExpecteResult = Result<[string, number, boolean], [string, number, boolean]>

const result: ExpecteResult = await ResultAsync.combineWithAllErrors(heterogenousList)

Expand Down Expand Up @@ -832,7 +826,6 @@ describe('ResultAsync', () => {
const okVal = okAsync(12)
const errorCallback = jest.fn((_errVal) => errAsync<number, string>('It is now a string'))


const result = await okVal.orElse(errorCallback)

expect(result).toEqual(ok(12))
Expand Down Expand Up @@ -959,7 +952,7 @@ describe('ResultAsync', () => {

return 12
})

const val = await example()
expect(val.isErr()).toBe(true)

Expand All @@ -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),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like prettier changes in this file.

)

const val = await example()
expect(val.isErr()).toBe(true)

Expand Down
26 changes: 0 additions & 26 deletions tests/tsconfig.tests.json
View file
Open in desktop

This file was deleted.

Loading
Loading