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

Jump to bottom

[@types/lodash] Infer lodash.throttle return type from 'leading' option #69504

Merged
merged 3 commits into from
May 16, 2024
Merged

[@types/lodash] Infer lodash.throttle return type from 'leading' option #69504

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c7c189e
infer lodash.throttle return type from 'leading' option
dawaltconley May 3, 2024
b00febf
fix throttle overloads to handle boolean variables and explicit undef…
dawaltconley May 3, 2024
4c69ba6
clean up tests
dawaltconley May 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 15 additions & 3 deletions types/lodash/common/function.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1355,6 +1355,9 @@ declare module "../index" {
*/
trailing?: boolean | undefined;
}
interface ThrottleSettingsNoLeading extends ThrottleSettings {
leading: false;
}
interface LoDashStatic {
/**
* Creates a throttled function that only invokes func at most once per every wait milliseconds. The throttled
Expand All @@ -1372,25 +1375,34 @@ declare module "../index" {
* @param options.trailing Specify invoking on the trailing edge of the timeout.
* @return Returns the new throttled function.
*/
throttle<T extends (...args: any) => any>(func: T, wait?: number, options?: ThrottleSettings): DebouncedFunc<T>;
throttle<T extends (...args: any) => any>(func: T, wait: number | undefined, options: ThrottleSettingsNoLeading): DebouncedFunc<T>;
throttle<T extends (...args: any) => any>(func: T, wait?: number, options?: ThrottleSettings): DebouncedFuncLeading<T>;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a little awkward, since this overload will match if the caller passes a boolean variable (instead of the literal this for leading. In this case, we don't know if it's leading or not, so ideally the return type should include undefined just in case.

Is there a way to check if the leading field is unset? e.g.

ThrottleSettingsLeadingUnset extends ThrottleSettings {
  leading?: never; // or maybe undefined?
}

Then return DebouncedFuncLeading only if leading is unset or set to true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! As an extra complication, I just checked and apparently lodash treats an explicit { leading: undefined } the same as { leading: false }. So unset and undefined fields should be treated differently.

This is what I came up with, going to commit with tests but not sure if there's a way to handle this without some kind of union type. never didn't seem to work.

interface ThrottleSettings {
    leading?: boolean | undefined;
    trailing?: boolean | undefined;
}
type ThrottleSettingsLeading = (ThrottleSettings & { leading: true }) | Omit<ThrottleSettings, 'leading'>
interface LoDashStatic {
    throttle<T extends (...args: any) => any>(func: T, wait?: number, options?: ThrottleSettingsLeading): DebouncedFuncLeading<T>;
    throttle<T extends (...args: any) => any>(func: T, wait?: number, options?: ThrottleSettings): DebouncedFunc<T>;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Wow good find! In that case it might be nice to discourage people from setting the field to undefined since it almost certainly doesn't do what they expect. But I don't think there's a good way to do that (at least not better than what you have already) so I think we'll have to settle for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just pushed a commit to clarify the tests. Had the thought right after that it is possible to prohibit an explicit undefined here, while still making leading an optional property.

interface ThrottleSettings {
    leading: boolean;
    trailing?: boolean | undefined;
}
type ThrottleSettingsLeading = (ThrottleSettings & { leading: true }) | Omit<ThrottleSettings, 'leading'>
interface LoDashStatic {
    throttle<T extends (...args: any) => any>(func: T, wait?: number, options?: ThrottleSettingsLeading): DebouncedFuncLeading<T>;
    throttle<T extends (...args: any) => any>(func: T, wait?: number, options?: ThrottleSettings): DebouncedFunc<T>;
}

But I think that's a breaking change and I don't know if it's preferable anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

Pretty much every type change is a breaking change, due to the way types work. But I think that change still doesn't completely prevent explicit undefined, so it's debatable whether it's worth the potential breakage. (It prevents explicit undefined in object literals but not prebuilt objects. Maybe better than nothing?)

}
interface Function<T extends (...args: any) => any> {
/**
* @see _.throttle
*/
throttle(
wait: number | undefined,
options: ThrottleSettingsNoLeading
): T extends (...args: any[]) => any ? Function<DebouncedFunc<T>> : never;
throttle(
wait?: number,
options?: ThrottleSettings
): T extends (...args: any[]) => any ? Function<DebouncedFunc<T>> : never;
): T extends (...args: any[]) => any ? Function<DebouncedFuncLeading<T>> : never;
}
interface FunctionChain<T extends (...args: any) => any> {
/**
* @see _.throttle
*/
throttle(
wait: number | undefined,
options: ThrottleSettingsNoLeading
): T extends (...args: any[]) => any ? FunctionChain<DebouncedFunc<T>> : never;
throttle(
wait?: number,
options?: ThrottleSettings
): T extends (...args: any[]) => any ? FunctionChain<DebouncedFunc<T>> : never;
): T extends (...args: any[]) => any ? FunctionChain<DebouncedFuncLeading<T>> : never;
}
interface LoDashStatic {
/**
Expand Down
6 changes: 3 additions & 3 deletions types/lodash/fp.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4173,10 +4173,10 @@ declare namespace _ {
interface LodashThrottle {
(wait: number): LodashThrottle1x1;
<T extends (...args: any) => any>(wait: lodash.__, func: T): LodashThrottle1x2<T>;
<T extends (...args: any) => any>(wait: number, func: T): lodash.DebouncedFunc<T>;
<T extends (...args: any) => any>(wait: number, func: T): lodash.DebouncedFuncLeading<T>;
}
type LodashThrottle1x1 = <T extends (...args: any) => any>(func: T) => lodash.DebouncedFunc<T>;
type LodashThrottle1x2<T extends (...args: any) => any> = (wait: number) => lodash.DebouncedFunc<T>;
type LodashThrottle1x1 = <T extends (...args: any) => any>(func: T) => lodash.DebouncedFuncLeading<T>;
type LodashThrottle1x2<T extends (...args: any) => any> = (wait: number) => lodash.DebouncedFuncLeading<T>;
interface LodashThru {
<T, TResult>(interceptor: (value: T) => TResult): LodashThru1x1<T, TResult>;
<T>(interceptor: lodash.__, value: T): LodashThru1x2<T>;
Expand Down
30 changes: 18 additions & 12 deletions types/lodash/lodash-tests.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3660,21 +3660,27 @@ fp.now(); // $ExpectType number
leading: true,
trailing: false,
};
const optionsNoLeading: _.ThrottleSettingsNoLeading = {
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be worth writing the options inline, instead of explicitly setting the type here, since most users won't set an explicit type, and that could mess with the overload selection.

leading: false,
};

const func = (a: number, b: string): boolean => true;

_.throttle(func); // $ExpectType DebouncedFunc<(a: number, b: string) => boolean>
_.throttle(func, 42); // $ExpectType DebouncedFunc<(a: number, b: string) => boolean>
_.throttle(func, 42, options); // $ExpectType DebouncedFunc<(a: number, b: string) => boolean>
_(func).throttle(); // $ExpectType Function<DebouncedFunc<(a: number, b: string) => boolean>>
_(func).throttle(42); // $ExpectType Function<DebouncedFunc<(a: number, b: string) => boolean>>
_(func).throttle(42, options); // $ExpectType Function<DebouncedFunc<(a: number, b: string) => boolean>>
_.chain(func).throttle(); // $ExpectType FunctionChain<DebouncedFunc<(a: number, b: string) => boolean>>
_.chain(func).throttle(42); // $ExpectType FunctionChain<DebouncedFunc<(a: number, b: string) => boolean>>
_.chain(func).throttle(42, options); // $ExpectType FunctionChain<DebouncedFunc<(a: number, b: string) => boolean>>

fp.throttle(42, func); // $ExpectType DebouncedFunc<(a: number, b: string) => boolean>
fp.throttle(42)(func); // $ExpectType DebouncedFunc<(a: number, b: string) => boolean>
_.throttle(func); // $ExpectType DebouncedFuncLeading<(a: number, b: string) => boolean>
_.throttle(func, 42); // $ExpectType DebouncedFuncLeading<(a: number, b: string) => boolean>
_.throttle(func, 42, options); // $ExpectType DebouncedFuncLeading<(a: number, b: string) => boolean>
_.throttle(func, 42, optionsNoLeading); // $ExpectType DebouncedFunc<(a: number, b: string) => boolean>
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a test with an empty object literal ({}) for the options, and a test where leading has type boolean (or true|false).

_(func).throttle(); // $ExpectType Function<DebouncedFuncLeading<(a: number, b: string) => boolean>>
_(func).throttle(42); // $ExpectType Function<DebouncedFuncLeading<(a: number, b: string) => boolean>>
_(func).throttle(42, options); // $ExpectType Function<DebouncedFuncLeading<(a: number, b: string) => boolean>>
_(func).throttle(42, optionsNoLeading); // $ExpectType Function<DebouncedFunc<(a: number, b: string) => boolean>>
_.chain(func).throttle(); // $ExpectType FunctionChain<DebouncedFuncLeading<(a: number, b: string) => boolean>>
_.chain(func).throttle(42); // $ExpectType FunctionChain<DebouncedFuncLeading<(a: number, b: string) => boolean>>
_.chain(func).throttle(42, options); // $ExpectType FunctionChain<DebouncedFuncLeading<(a: number, b: string) => boolean>>
_.chain(func).throttle(42, optionsNoLeading); // $ExpectType FunctionChain<DebouncedFunc<(a: number, b: string) => boolean>>

fp.throttle(42, func); // $ExpectType DebouncedFuncLeading<(a: number, b: string) => boolean>
fp.throttle(42)(func); // $ExpectType DebouncedFuncLeading<(a: number, b: string) => boolean>
}

// _.unary
Expand Down