-
Notifications
You must be signed in to change notification settings - Fork 12
feat: improve zipWidth element count #210
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new multi-Maybe zipWith overloads and implementation are logically sound and well covered by tests, but there are a couple of edge cases where the runtime and type-level contracts can diverge from the documented intent. The runtime implementation of zipWith assumes the final argument is a function and that at least one other Maybe is passed, which is only enforced by TypeScript and could fail in looser-typed usage. The variadic overload in the interface currently allows calls without any additional Maybe arguments, contradicting the JSDoc description that zipWith combines with "one or more" other Maybes. Tightening these aspects would improve robustness and clarity without changing the core behavior.
Summary of changes
Summary of Changes
- Expanded the
IMaybe#zipWithinterface from a binary combinator to multiple overloads supporting 2–6Maybevalues, plus a variadic fallback overload. - Updated JSDoc comments and examples on
zipWithto describe multi-Maybebehavior and added multi-argument usage examples. - Implemented a new variadic
zipWithmethod inMaybe<T>that:- Early-returns
Noneifthisor any providedMaybeisNone. - Collects all values (including
this) and applies the final function argument.
- Early-returns
- Added comprehensive tests in
maybe.spec.tscovering combinations of 3–6 Maybes, including mixed-type and partial-Nonescenarios, ensuring the newzipWithbehavior is validated. - Minor whitespace cleanups in the tests.
| public zipWith<R>(...args: unknown[]): IMaybe<R> { | ||
| if (this.isNone()) { | ||
| return new Maybe<R>() | ||
| } | ||
|
|
||
| const fn = args[args.length - 1] as (...values: unknown[]) => NonNullable<R> | ||
| const maybes = args.slice(0, -1) as IMaybe<unknown>[] | ||
|
|
||
| const values: unknown[] = [this.value] | ||
|
|
||
| for (const m of maybes) { | ||
| if (m.isNone()) { | ||
| return new Maybe<R>() | ||
| } | ||
| values.push(m.valueOrThrow()) | ||
| } | ||
|
|
||
| return new Maybe<R>(fn(...values)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation assumes that the last argument is always a function and that there is at least one other IMaybe provided, but these invariants are only enforced by the TypeScript overloads.
At runtime, if zipWith is ever called with:
- Only a function (
a.zipWith(fn)), or - No trailing function (e.g. mis-usage via
anyor loose typing),
fn here will either be undefined or a non-function and fn(...values) below will throw. While the types should prevent this in well-typed code, this is a low-level core utility where defensive checks are relatively cheap and can fail fast with a clearer error.
Given the library appears to aim for robustness in other places (e.g. flatMapMany catch returning None), a minimal runtime validation would improve resilience and debuggability, especially for consumers using any or JS interop.
Suggestion
Consider adding minimal runtime validation for the function and argument count, for example:
public zipWith<R>(...args: unknown[]): IMaybe<R> {
if (this.isNone()) {
return new Maybe<R>()
}
if (args.length === 0) {
throw new Error('zipWith requires at least one other Maybe and a combiner function')
}
const fn = args[args.length - 1]
if (typeof fn !== 'function') {
throw new Error('zipWith last argument must be a function')
}
const maybes = args.slice(0, -1) as IMaybe<unknown>[]
const values: unknown[] = [this.value]
for (const m of maybes) {
if (m.isNone()) {
return new Maybe<R>()
}
values.push(m.valueOrThrow())
}
return new Maybe<R>((fn as (...values: unknown[]) => Nonnullable<R>)(...values))
}This keeps the overload behavior intact while offering a clearer failure mode when misused from less strict code. Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| // Variadic overload for 5+ Maybes | ||
| zipWith<R>( | ||
| ...args: [...IMaybe<NonNullable<unknown>>[], (...values: NonNullable<unknown>[]) => NonNullable<R>] | ||
| ): IMaybe<R> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current variadic overload type:
zipWith<R>(
...args: [...IMaybe<NonNullable<unknown>>[], (...values: NonNullable<unknown>[]) => NonNullable<R>]
): IMaybe<R>implicitly allows calling zipWith without any other Maybes, e.g. maybe(1).zipWith(x => x + 1) at the type level (the tuple [] is a valid IMaybe<NonNullable<unknown>>[]). This contradicts the JSDoc examples and the intended semantics of “Combines this Maybe with one or more other Maybes”.
This mismatch between the documented contract and the type-level contract can lead to confusing or unsafe usage, especially given the generic unknown-based typing here. It would be better to encode “at least one other Maybe” in the signature itself.
Suggestion
You can enforce the presence of at least one additional Maybe in the variadic overload by changing the tuple rest pattern, for example:
zipWith<R>(
...args: [IMaybe<NonNullable<unknown>>, ...IMaybe<NonNullable<unknown>>[], (...values: NonNullable<unknown>[]) => NonNullable<R>]
): IMaybe<R>This preserves the existing overloads for up to five extra Maybes while making the fallback overload consistent with the documented requirement of “one or more other Maybes.” Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| // Variadic overload for 5+ Maybes | ||
| zipWith<R>( | ||
| ...args: [...IMaybe<NonNullable<unknown>>[], (...values: NonNullable<unknown>[]) => NonNullable<R>] | ||
| ): IMaybe<R> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new overloads cover arities 2–6 plus a variadic signature, which is powerful but also introduces a somewhat tricky call contract: callers must always pass n Maybes plus one final combiner function. The implementation in Maybe#zipWith assumes the last argument is the function and does no runtime validation, so a mis-ordered call will silently manifest as a logic bug rather than a clear error.
Given how overloaded this API has become, it might be worth tightening the public surface a bit:
- Either clearly document in the interface comment that the last argument must be the combiner function and that all preceding arguments must be
IMaybes, or - Consider removing the variadic overload and capping at the explicit 2–6 overloads for stricter type-checked usage.
Right now consumers could still do something like a.zipWith(b as any, c, d) and hit surprising runtime behavior without any guardrails.
Suggestion
You could tighten the contract and improve debuggability either by documenting the calling convention explicitly or by validating the last argument at runtime. For example, a lightweight runtime guard:
zipWith<R>(
...args: [...IMaybe<NonNullable<unknown>>[], (...values: NonNullable<unknown>[]) => NonNullable<R>]
): IMaybe<R>;and in the implementation:
public zipWith<R>(...args: unknown[]): IMaybe<R> {
if (this.isNone()) {
return new Maybe<R>();
}
const fn = args[args.length - 1];
if (typeof fn !== 'function') {
throw new TypeError('zipWith: last argument must be a function');
}
const maybes = args.slice(0, -1) as IMaybe<unknown>[];
// ...rest stays the same
}This keeps the strong typing while turning obvious misuses into clear runtime errors instead of silent mis-computation. Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
No description provided.