@sirbrillig Thank you for submitting this PR!

This is a live comment which I will keep updated.

1 package in this PR

• wordpress__data — on npm, on unpkg

Code Reviews

Because you edited one package and updated the tests (👏), I can help you merge this PR once someone else signs off on it.

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 type definition owners, DT maintainers or others

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.

{
  "type": "info",
  "now": "-",
  "pr_number": 60693,
  "author": "sirbrillig",
  "headCommitOid": "66189e99b4fbbaf7f28f07b5e533451b695c8e31",
  "mergeBaseOid": "595c471bd4aefa44a5d0ee49c4d73f373c319d18",
  "lastPushDate": "2022-06-08T22:28:20.000Z",
  "lastActivityDate": "2022-06-20T19:32:54.000Z",
  "mergeOfferDate": "2022-06-16T16:24:22.000Z",
  "mergeRequestDate": "2022-06-20T19:32:54.000Z",
  "mergeRequestUser": "sirbrillig",
  "hasMergeConflict": false,
  "isFirstContribution": false,
  "tooManyFiles": false,
  "hugeChange": false,
  "popularityLevel": "Well-liked by everyone",
  "pkgInfo": [
    {
      "name": "wordpress__data",
      "kind": "edit",
      "files": [
        {
          "path": "types/wordpress__data/index.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/wordpress__data/wordpress__data-tests.tsx",
          "kind": "test"
        }
      ],
      "owners": [
        "dsifford",
        "sirreal"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Well-liked by everyone"
    }
  ],
  "reviews": [
    {
      "type": "approved",
      "reviewer": "dmsnell",
      "date": "2022-06-16T16:23:34.000Z",
      "isMaintainer": false
    },
    {
      "type": "stale",
      "reviewer": "sarayourfriend",
      "date": "2022-06-07T21:03:08.000Z",
      "abbrOid": "6353f1f"
    },
    {
      "type": "stale",
      "reviewer": "sirreal",
      "date": "2022-06-06T19:34:21.000Z",
      "abbrOid": "e4798c4"
    }
  ],
  "mainBotCommentID": 1147822909,
  "ciResult": "pass"
}

🔔 @dsifford @sirreal — 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.

@sirbrillig The CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

Note: builds which are failing do not end up on the list of PRs for the DT maintainers to review.

Inspecting the JavaScript source for this package found some properties that are not in the .d.ts files.
The check for missing properties isn't always right, so take this list as advice, not a requirement.

wordpress__data (unpkg)

was missing the following properties:

1. AsyncModeProvider
2. controls
3. AsyncModeProvider
4. controls

Generated by 🚫 dangerJS against 66189e9

@sirreal 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?

Pinging also @dmsnell and @jsnajdr because you have much more TS experience with this package than I do.

Updated to make this less breaking thanks to an excellent idea by @sarayourfriend.

@sirbrillig The CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

Note: builds which are failing do not end up on the list of PRs for the DT maintainers to review.

@sirbrillig The CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

Note: builds which are failing do not end up on the list of PRs for the DT maintainers to review.

@sirbrillig The CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

Note: builds which are failing do not end up on the list of PRs for the DT maintainers to review.

@sarayourfriend, @sirreal 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?

Would this be simpler if we relaxed the type parameters?

Record<string, <T extends Promise<any> | any>(...args: readonly any[]) => T>

this captures the Promise nature without any further work on our part. I think even <T> is fine without the extends other than it communicates to devs that Promise<> is likely

Would this be simpler if we relaxed the type parameters?

Making it simpler sounds nice, but when I try using that type in actual code, the type of an action without an explicit generic becomes const doSomething: <unknown>(...args: readonly any[]) => unknown and if I try to add .then() I get a type error because unknown has no properties.

For reference, here's the code I'm using to test these changes. Currently it has an explicit generic, but IMO it should work without a generic at all because the default behavior is to return a Promise.

@sirbrillig: Everything looks good here. I am ready to merge this PR (at 66189e9) 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! ❤️

(@dsifford, @sirreal: you can do this too.)

Ready to merge