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.

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

Add non-spread overload for Result.all #125

Merged
merged 2 commits into from
Apr 24, 2024

Conversation

ConnorSinnott
Copy link

@ConnorSinnott ConnorSinnott commented Apr 16, 2024

Howdy! I'm bringing this PR over from the decidedly dead vultix/ts-results repo.

This PR adds non-spread variants of Result.all and Result.any which should be preferred over the parameter spread variants. For instances where Result.all or Result.any are invoked with very large arrays, the spread operation could lead to stack overflows.

Additionally, speaking from personal experience in an enterprise environment; the usage of Result.all via a spread array (Result.all(...myResults)) grossly exceeds the usage of Result.all with multiple parameters. I'd imagine this is the same for others as well.

This new behavior has been added as an overload of Result.all to offer backwards compatibility for those spreading arrays in, or passing each argument individually. Though based on the stack overflow issue mentioned above, I'd recommend users migrate from array spreading to just passing the array.

// Utility types
type Head<T extends any[]> = T extends [any, ...infer R] ?
T extends [...infer F, ...R] ? F : never : never
type Tail<T extends any[]> = T extends [any, ...infer R] ? R : never
Copy link
Author

Choose a reason for hiding this comment

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

These could likely be removed since the arrays I'm parsing above are typed as any[]. I could just as easily type the arguments as arg0: T[0] | T, argN: ...T but using these types are technically truer to what's going on.

jest.config.js Outdated
'^.*\.ts$': ['ts-jest', {
tsconfig: 'test/tsconfig.json'
}]
},
Copy link
Author

Choose a reason for hiding this comment

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

This silences the warnings that global ts-jest configuration is deprecated

@jstasiak
Copy link
Collaborator

Hey, I think this is a nice idea, thank you very much for this. I'll do a full review later but in the meantime please can you remove unrelated changes from this PR? (reformatting, whitespace, jest.config changes, anything else that I missed)

@ConnorSinnott ConnorSinnott force-pushed the add-non-spread-result-all branch 2 times, most recently from 08f3d0b to 6141535 Compare April 17, 2024 15:42
@ConnorSinnott
Copy link
Author

Done! Thank you @jstasiak. Had to wrestle with the editor to omit the whitespace changes. All tests pass now as well.

@ConnorSinnott
Copy link
Author

The jest changes have been moved to #126

@jstasiak
Copy link
Collaborator

Thank you, I think this is a great addition.

Can you update the documentation too? It's in docs/reference/api/result.rst, let me know if you don't know Sphinx or reStructuredText or otherwise need assistance with this.

@ConnorSinnott ConnorSinnott force-pushed the add-non-spread-result-all branch 6 times, most recently from 440f751 to f6035ce Compare April 23, 2024 20:52
@ConnorSinnott
Copy link
Author

Ok @jstasiak, how do those docs read for you?

@jstasiak
Copy link
Collaborator

Very nice, thank you

@jstasiak jstasiak merged commit 2e03308 into lune-climate:master Apr 24, 2024
2 checks passed
@ConnorSinnott ConnorSinnott deleted the add-non-spread-result-all branch April 24, 2024 17:09
@jstasiak
Copy link
Collaborator

Released in version 4.2.0 just now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants