[@types/lodash] Infer lodash.throttle return type from 'leading' option #69504
Conversation
|
@dawaltconley Thank you for submitting this PR! This is a live comment which I will keep updated. 1 package in this PRCode ReviewsBecause this is a widely-used package, a DT maintainer will need to review it before it can be merged. You can test the changes of this PR in the Playground. Status
All of the items on the list are green. To merge, you need to post a comment including the string "Ready to merge" to bring in your changes. Diagnostic Information: What the bot saw about this PR{
"type": "info",
"now": "-",
"pr_number": 69504,
"author": "dawaltconley",
"headCommitOid": "4c69ba689494218c99c8390ce79c25ed6976fa22",
"mergeBaseOid": "1243e1a4d11bb863290220ddb6fd678526bf4d47",
"lastPushDate": "2024-05-03T00:48:47.000Z",
"lastActivityDate": "2024-05-16T15:02:42.000Z",
"mergeOfferDate": "2024-05-16T14:04:38.000Z",
"mergeRequestDate": "2024-05-16T15:02:42.000Z",
"mergeRequestUser": "dawaltconley",
"hasMergeConflict": false,
"isFirstContribution": false,
"tooManyFiles": false,
"hugeChange": false,
"popularityLevel": "Critical",
"pkgInfo": [
{
"name": "lodash",
"kind": "edit",
"files": [
{
"path": "types/lodash/common/function.d.ts",
"kind": "definition"
},
{
"path": "types/lodash/fp.d.ts",
"kind": "definition"
},
{
"path": "types/lodash/lodash-tests.ts",
"kind": "test"
}
],
"owners": [
"bczengel",
"chrootsu",
"aj-r",
"e-cloud",
"thorn0",
"jtmthf",
"DomiR",
"WilliamChelman"
],
"addedOwners": [],
"deletedOwners": [],
"popularityLevel": "Critical"
}
],
"reviews": [
{
"type": "approved",
"reviewer": "sandersn",
"date": "2024-05-16T14:04:01.000Z",
"isMaintainer": true
},
{
"type": "approved",
"reviewer": "aj-r",
"date": "2024-05-14T17:48:58.000Z",
"isMaintainer": false
}
],
"mainBotCommentID": 2091965284,
"ciResult": "pass"
} |
|
🔔 @bczengel @chrootsu @aj-r @e-cloud @thorn0 @jtmthf @DomiR @WilliamChelman — please review this PR in the next few days. Be sure to explicitly select |
typescript-bot
moved this from Waiting for Code Reviews
to Needs Maintainer Review
in New Pull Request Status Board
types/lodash/common/function.d.ts
Outdated
| @@ -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>; | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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>;
}There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?)
types/lodash/lodash-tests.ts
Outdated
| @@ -3660,21 +3660,27 @@ fp.now(); // $ExpectType number | |||
| leading: true, | |||
| trailing: false, | |||
| }; | |||
| const optionsNoLeading: _.ThrottleSettingsNoLeading = { | |||
There was a problem hiding this comment.
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.
types/lodash/lodash-tests.ts
Outdated
| _.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> |
There was a problem hiding this comment.
Add a test with an empty object literal ({}) for the options, and a test where leading has type boolean (or true|false).
typescript-bot
added
the
Revision needed
typescript-bot
moved this from Needs Maintainer Review
to Needs Author Action
in New Pull Request Status Board
|
@dawaltconley One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits. Thank you! |
typescript-bot
removed
the
Revision needed
typescript-bot
moved this from Needs Author Action
to Waiting for Code Reviews
in New Pull Request Status Board
|
@aj-r Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review? |
typescript-bot
moved this from Waiting for Code Reviews
to Needs Maintainer Review
in New Pull Request Status Board
types/lodash/lodash-tests.ts
Outdated
| }; | ||
| const boolean: boolean = false; |
There was a problem hiding this comment.
nit: you might need false as boolean, otherwise TS will automatically narrow the type to false which is not what we want.
types/lodash/lodash-tests.ts
Outdated
| }; | ||
| const boolean: boolean = false; | ||
| const maybeUndefined = (function (): true | undefined { return true })(); |
There was a problem hiding this comment.
Similar to above: a simpler way to do this might be something like:
const maybeUndefined: true | undefined = true as any;
typescript-bot
added
the
Owner Approved
types/lodash/lodash-tests.ts
Outdated
| _.throttle(func, 42, { leading: false }); // $ExpectType DebouncedFunc<(a: number, b: string) => boolean> | ||
| _.throttle(func, 42, { leading: undefined }); // $ExpectType DebouncedFunc<(a: number, b: string) => boolean> | ||
| _.throttle(func, 42, { leading: boolean }); // $ExpectType DebouncedFunc<(a: number, b: string) => boolean> | ||
| _.throttle(func, 42, { leading: maybeUndefined }); // $ExpectType DebouncedFunc<(a: number, b: string) => boolean> |
There was a problem hiding this comment.
Also add a comment explaining why we should NOT expect DebouncedFuncLeading here, since this is probably surprising to everyone.
typescript-bot
added
Revision needed
|
@dawaltconley One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits. Thank you! |
typescript-bot
moved this from Needs Maintainer Review
to Needs Author Action
in New Pull Request Status Board
Explicitly casts ambiguous values to avoid type narrowing. Also adds explanatory comments to tests with an explicit undefined leading.
typescript-bot
removed
the
Revision needed
typescript-bot
moved this from Needs Author Action
to Waiting for Code Reviews
in New Pull Request Status Board
|
@aj-r Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review? |
typescript-bot
moved this from Waiting for Code Reviews
to Needs Maintainer Review
in New Pull Request Status Board
typescript-bot
added
the
Owner Approved
typescript-bot
removed
the
Owner Approved
typescript-bot
added
the
Unreviewed
typescript-bot
added
the
Owner Approved
typescript-bot
added
Maintainer Approved
Self Merge
|
@dawaltconley: Everything looks good here. I am ready to merge this PR (at 4c69ba6) on your behalf whenever you think it's ready. If you'd like that to happen, please post a comment saying:
and I'll merge this PR almost instantly. Thanks for helping out! ❤️ (@bczengel, @chrootsu, @aj-r, @e-cloud, @thorn0, @jtmthf, @DomiR, @WilliamChelman: you can do this too.) |
typescript-bot
moved this from Needs Maintainer Review
to Waiting for Author to Merge
in New Pull Request Status Board
|
Ready to merge |
typescript-bot
moved this from Waiting for Author to Merge
to Recently Merged
in New Pull Request Status Board
This sets the
throttletypes to follow the same pattern asdebounce(which it calls directly), accounting for the different default values (throttle hasleading: trueby default, debounce hasleading: false).I'm less familiar with how the
fppackages are supposed to work, didn't see a way to pass options. I changed thelodash/fp/throttlefunction to return the same value aslodash/throttlewith default options (DebounceFuncLeading).Please fill in this template.
pnpm test <package to test>.Select one of these and delete the others:
If changing an existing definition:
package.json.