[lodash] Fix _.partial() with 3 arity func and full args #69561

YuseiUeno · 2024-05-10T05:14:18Z

Please fill in this template.

Use a meaningful title for the pull request. Include the name of the package modified.
Test the change in your own code. (Compile and run.)
Add or edit tests to reflect the change.
Follow the advice from the readme.
Avoid common mistakes.
Run pnpm test <package to test>.

Select one of these and delete the others:

If changing an existing definition:

Provide a URL to documentation or source code which provides context for the suggested changes: https://lodash.com/docs/4.17.15#partial
If this PR brings the type definitions up to date with a new version of the JS library, update the version number in the package.json.

https://www.typescriptlang.org/play/?ssl=13&ssc=9&pln=12&pc=1#code/JYWwDg9gTgLgBAfQDRwN5wGIFcB2BjGYCHFAGQgBEBDAZwAsBlGKwvOAXzgDMoIQ4A5ABsIAE1p0BAWABQsvMRrwuARjgBeOAAoqALjg4sIAEYBTKAEoNAPjhUA3PMXKATBu16DRs1BTH9SlDAOADmVuq2VHAA1HDGjjIKOErcAMzuOvqGJuZ+ATBBoSh4+sYQEEKmVDjhkTFx9XgJSSlcACwZntk+eXCBwSHFpeWV1SiiWd7mtXb1xo31oglOyfBgapoIAHRgVLDAVEIAPADaALoo3bleOVDWWqooKgAMFgkA9O9w33AAegD8KxSYDcmx2e0IhweLiezxQAioxjwAjesk+Pz+gMSzjgYHSYN2+yhXFSsPhiORKAKWFMqJk6J+AKBaw6BIhByEDzaZMEFIEVKgNNhdIZ3yZMiAA

p3 must be () => string. But below.

const p3 = _.partial(f3, 10, 'abc', true);
//    ^? const p3: _.Function1<unknown, string>

<T1, T2, T3, T4, R>(func: Function4<T1, T2, T3, T4, R>, arg1: T1, arg2: T2, arg3: T3): Function1<T4, R>; brake <TS extends any[], T1, T2, T3, R>(func: (t1: T1, t2: T2, t3: T3, ...ts: TS) => R, t1: T1, t2: T2, t3: T3): (...ts: TS) => R; by overload

typescript-bot · 2024-05-10T05:15:12Z

@YuseiUeno Thank you for submitting this PR!

This is a live comment which I will keep updated.

1 package in this PR

lodash — on npm, on unpkg
- owner-approval: @aj-r

Code Reviews

Because 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

✅ No merge conflicts
✅ Continuous integration tests have passed
✅ Most recent commit is approved by a DT maintainer

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": 69561,
  "author": "YuseiUeno",
  "headCommitOid": "d293cb430e2778a9f2446ddd07e7f6eb8f967cba",
  "mergeBaseOid": "18753976aad3956a963c3cafeeab6755b7c879a3",
  "lastPushDate": "2024-05-10T05:14:18.000Z",
  "lastActivityDate": "2024-05-16T14:07:23.000Z",
  "mergeOfferDate": "2024-05-16T14:02:52.000Z",
  "mergeRequestDate": "2024-05-16T14:07:23.000Z",
  "mergeRequestUser": "YuseiUeno",
  "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/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:02:15.000Z",
      "isMaintainer": true
    },
    {
      "type": "approved",
      "reviewer": "aj-r",
      "date": "2024-05-13T10:49:50.000Z",
      "isMaintainer": false
    }
  ],
  "mainBotCommentID": 2103880510,
  "ciResult": "pass"
}

typescript-bot · 2024-05-10T05:15:13Z

🔔 @bczengel @chrootsu @aj-r @e-cloud @thorn0 @jtmthf @DomiR @WilliamChelman — please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

aj-r

I don't think this change is necessary. When I test your example, it works as expected: https://stackblitz.com/edit/typescript-lodash-playground-1tpr1g?file=index.ts. In that code, p3 has type Function0<string> as expected.

Perhaps the issue is that your f3 has multiple overloads with different numbers of arguments? If so, you probably want to avoid partial since its behavior will be ambiguous.

Thanks for adding the missing tests though - optionally you could just revert the change to function.d.ts and sumbit the new tests.

aj-r · 2024-05-12T10:54:32Z

types/lodash/common/function.d.ts

@@ -655,7 +655,6 @@ declare module "../index" {
        <T1, T2, T3, T4, R>(func: Function4<T1, T2, T3, T4, R>, plc1: __, plc2: __, arg3: T3): Function3<T1, T2, T4, R>;
        <T1, T2, T3, T4, R>(func: Function4<T1, T2, T3, T4, R>, arg1: T1, plc2: __, arg3: T3): Function2<T2, T4, R>;
        <T1, T2, T3, T4, R>(func: Function4<T1, T2, T3, T4, R>, plc1: __, arg2: T2, arg3: T3): Function2<T1, T4, R>;
-        <T1, T2, T3, T4, R>(func: Function4<T1, T2, T3, T4, R>, arg1: T1, arg2: T2, arg3: T3): Function1<T4, R>;


This seems to be working as intended. I don't think it should be removed.

Thank you for checking my code!
Really?
It looks not working with pnpm test.

❯ git --no-pager diff --cached diff --git a/types/lodash/common/function.d.ts b/types/lodash/common/function.d.ts index 46ac3b3746..7628278464 100644 --- a/types/lodash/common/function.d.ts +++ b/types/lodash/common/function.d.ts @@ -655,6 +655,7 @@ declare module "../index" { <T1, T2, T3, T4, R>(func: Function4<T1, T2, T3, T4, R>, plc1: __, plc2: __, arg3: T3): Function3<T1, T2, T4, R>; <T1, T2, T3, T4, R>(func: Function4<T1, T2, T3, T4, R>, arg1: T1, plc2: __, arg3: T3): Function2<T2, T4, R>; <T1, T2, T3, T4, R>(func: Function4<T1, T2, T3, T4, R>, plc1: __, arg2: T2, arg3: T3): Function2<T1, T4, R>; + <T1, T2, T3, T4, R>(func: Function4<T1, T2, T3, T4, R>, arg1: T1, arg2: T2, arg3: T3): Function1<T4, R>; <T1, T2, T3, T4, R>(func: Function4<T1, T2, T3, T4, R>, plc1: __, plc2: __, plc3: __, arg4: T4): Function3<T1, T2, T3, R>; <T1, T2, T3, T4, R>(func: Function4<T1, T2, T3, T4, R>, arg1: T1, plc2: __, plc3: __, arg4: T4): Function2<T2, T3, R>; <T1, T2, T3, T4, R>(func: Function4<T1, T2, T3, T4, R>, plc1: __, arg2: T2, plc3: __, arg4: T4): Function2<T1, T3, R>;

❯ pnpm -w run test lodash > definitely-typed@0.0.3 test /Users/yusei/Workspace/repos/github.com/DefinitelyTyped/DefinitelyTyped > node --enable-source-maps node_modules/@definitelytyped/dtslint/ types "lodash" dtslint@0.2.20 Error: /Users/yusei/Workspace/repos/github.com/DefinitelyTyped/DefinitelyTyped/types/lodash/lodash-tests.ts 7304:5 error TypeScript@4.7, 4.8, 4.9, 5.0, 5.1, 5.2, 5.3, 5.4, 5.5 expected type to be: () => number got: Function1<unknown, number> @definitelytyped/expect ✖ 1 problem (1 error, 0 warnings) at combineErrorsAndWarnings (/Users/yusei/Workspace/repos/github.com/DefinitelyTyped/DefinitelyTyped/node_modules/.pnpm/@definitelytyped+dtslint@0.2.20_typescript@5.5.0-dev.20240509/node_modules/@definitelytyped/dtslint/src/index.ts:259:26) at runTests (/Users/yusei/Workspace/repos/github.com/DefinitelyTyped/DefinitelyTyped/node_modules/.pnpm/@definitelytyped+dtslint@0.2.20_typescript@5.5.0-dev.20240509/node_modules/@definitelytyped/dtslint/src/index.ts:250:18) at process.processTicksAndRejections (node:internal/process/task_queues:95:5) at main (/Users/yusei/Workspace/repos/github.com/DefinitelyTyped/DefinitelyTyped/node_modules/.pnpm/@definitelytyped+dtslint@0.2.20_typescript@5.5.0-dev.20240509/node_modules/@definitelytyped/dtslint/src/index.ts:103:22) ELIFECYCLE Test failed. See above for more details.

And this pattern is followed here so I delete it.

This pattern

<T1, T2, T3, T4, R>(func: Function4<T1, T2, T3, T4, R>, arg1: T1, arg2: T2, arg3: T3): Function1<T4, R>;

follow by

DefinitelyTyped/types/lodash/common/function.d.ts

Line 669 in d293cb4

<TS extends any[], T1, T2, T3, T4, R>(func: (t1: T1, t2: T2, t3: T3, t4: T4, ...ts: TS) => R, t1: T1, t2: T2, t3: T3, t4: T4): (...ts: TS) => R;

Please check works as not expected:https://stackblitz.com/edit/fjjtuv?file=index.ts

May be your stackblitz dependency is too old.

Yours

Mine

Ah, yeah I missed the overload below which does this. Thanks!

aj-r · 2024-05-12T10:55:40Z

types/lodash/lodash-tests.ts

+    _.partial(func3) // $ExpectType (arg1: number, arg2: string, arg3: boolean) => number
+    _.partial(func3, 42) // $ExpectType (arg2: string, arg3: boolean) => number
+    _.partial(func3, 42,     _, true) // $ExpectType Function1<string, number>;
+    _.partial(func3, 42, "foo", true) // $ExpectType () => number


This test essentially passes even without your changes (although the expected type would be Function0<number> which is basicalyl the same).

typescript-bot · 2024-05-12T10:59:54Z

@YuseiUeno 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!

YuseiUeno · 2024-05-13T00:04:31Z

<T1, T2, T3, T4, R>(func: Function4<T1, T2, T3, T4, R>, arg1: T1, arg2: T2, arg3: T3): Function1<T4, R>; brake <TS extends any[], T1, T2, T3, R>(func: (t1: T1, t2: T2, t3: T3, ...ts: TS) => R, t1: T1, t2: T2, t3: T3): (...ts: TS) => R; by overload

This is Function Overloads Ordering Problem.
https://github.com/microsoft/TypeScript-Handbook/blob/master/pages/declaration%20files/Do's%20and%20Don'ts.md#ordering

aj-r · 2024-05-13T10:49:42Z

types/lodash/common/function.d.ts

@@ -655,7 +655,6 @@ declare module "../index" {
        <T1, T2, T3, T4, R>(func: Function4<T1, T2, T3, T4, R>, plc1: __, plc2: __, arg3: T3): Function3<T1, T2, T4, R>;
        <T1, T2, T3, T4, R>(func: Function4<T1, T2, T3, T4, R>, arg1: T1, plc2: __, arg3: T3): Function2<T2, T4, R>;
        <T1, T2, T3, T4, R>(func: Function4<T1, T2, T3, T4, R>, plc1: __, arg2: T2, arg3: T3): Function2<T1, T4, R>;
-        <T1, T2, T3, T4, R>(func: Function4<T1, T2, T3, T4, R>, arg1: T1, arg2: T2, arg3: T3): Function1<T4, R>;


Ah, yeah I missed the overload below which does this. Thanks!

typescript-bot · 2024-05-16T14:02:52Z

@YuseiUeno: Everything looks good here. I am ready to merge this PR (at d293cb4) on your behalf whenever you think it's ready.

If you'd like that to happen, please post a comment saying:

Ready to merge

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.)

YuseiUeno · 2024-05-16T14:07:23Z

Ready to merge

fix: partial with 3 arity func and full args

d293cb4

typescript-bot added the Critical package label May 10, 2024

typescript-bot added this to Waiting for Code Reviews in New Pull Request Status Board May 10, 2024

typescript-bot moved this from Waiting for Code Reviews to Needs Maintainer Review in New Pull Request Status Board May 10, 2024

YuseiUeno changed the title ~~[lodash] Fix partial with 3 arity func and full args~~ [lodash] Fix _.partial() with 3 arity func and full args May 10, 2024

aj-r suggested changes May 12, 2024

View reviewed changes

typescript-bot added the Revision needed This PR needs code changes before it can be merged. label May 12, 2024

typescript-bot moved this from Needs Maintainer Review to Needs Author Action in New Pull Request Status Board May 12, 2024

YuseiUeno requested a review from aj-r May 13, 2024 10:00

aj-r approved these changes May 13, 2024

View reviewed changes

typescript-bot added Owner Approved A listed owner of this package signed off on the pull request. and removed Revision needed This PR needs code changes before it can be merged. labels May 13, 2024

typescript-bot moved this from Needs Author Action to Needs Maintainer Review in New Pull Request Status Board May 13, 2024

sandersn approved these changes May 16, 2024

View reviewed changes

typescript-bot added Maintainer Approved Self Merge This PR can now be self-merged by the PR author or an owner labels May 16, 2024

typescript-bot moved this from Needs Maintainer Review to Waiting for Author to Merge in New Pull Request Status Board May 16, 2024

typescript-bot moved this from Waiting for Author to Merge to Recently Merged in New Pull Request Status Board May 16, 2024

typescript-bot merged commit 7fb3bcc into DefinitelyTyped:master May 16, 2024
7 checks passed

YuseiUeno deleted the fix-lodash-partial branch May 16, 2024 14:09

typescript-bot removed this from Recently Merged in New Pull Request Status Board May 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[lodash] Fix _.partial() with 3 arity func and full args #69561

[lodash] Fix _.partial() with 3 arity func and full args #69561

YuseiUeno commented May 10, 2024 •

edited

typescript-bot commented May 10, 2024 •

edited

typescript-bot commented May 10, 2024

aj-r left a comment

aj-r May 12, 2024

YuseiUeno May 12, 2024 •

edited

YuseiUeno May 12, 2024 •

edited

YuseiUeno May 12, 2024 •

edited

aj-r May 13, 2024

aj-r May 12, 2024

typescript-bot commented May 12, 2024

YuseiUeno commented May 13, 2024

aj-r May 13, 2024

typescript-bot commented May 16, 2024

YuseiUeno commented May 16, 2024

[lodash] Fix _.partial() with 3 arity func and full args #69561

[lodash] Fix _.partial() with 3 arity func and full args #69561

Conversation

YuseiUeno commented May 10, 2024 • edited

typescript-bot commented May 10, 2024 • edited

1 package in this PR

Code Reviews

Status

typescript-bot commented May 10, 2024

aj-r left a comment

Choose a reason for hiding this comment

aj-r May 12, 2024

Choose a reason for hiding this comment

YuseiUeno May 12, 2024 • edited

Choose a reason for hiding this comment

YuseiUeno May 12, 2024 • edited

Choose a reason for hiding this comment

YuseiUeno May 12, 2024 • edited

Choose a reason for hiding this comment

aj-r May 13, 2024

Choose a reason for hiding this comment

aj-r May 12, 2024

Choose a reason for hiding this comment

typescript-bot commented May 12, 2024

YuseiUeno commented May 13, 2024

aj-r May 13, 2024

Choose a reason for hiding this comment

typescript-bot commented May 16, 2024

YuseiUeno commented May 16, 2024

YuseiUeno commented May 10, 2024 •

edited

typescript-bot commented May 10, 2024 •

edited

YuseiUeno May 12, 2024 •

edited

YuseiUeno May 12, 2024 •

edited

YuseiUeno May 12, 2024 •

edited