DefinitelyTyped icon indicating copy to clipboard operation
DefinitelyTyped copied to clipboard

[@types/chrome] Fix chrome.tabs.sendMessage()

Open RC-14 opened this issue 2 years ago • 3 comments

Reflect that chrome.tabs.sendMessage() returns a Promise if the optional callback parameter is not set.

If changing an existing definition:

  • [x] Provide a URL to documentation or source code which provides context for the suggested changes: https://developer.chrome.com/docs/extensions/reference/tabs/#method-sendMessage
  • [ ] If this PR brings the type definitions up to date with a new version of the JS library, update the version number in the header.

RC-14 avatar Aug 10 '22 14:08 RC-14

@RC-14 Thank you for submitting this PR! I see this is your first time submitting to DefinitelyTyped 👋 — I'm the local bot who will help you through the process of getting things through.

This is a live comment which I will keep updated.

1 package in this PR

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 or DT maintainers

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": 61644,
  "author": "RC-14",
  "headCommitOid": "3aa30da504f09ce5bb992b9609ea0169e9ad7dea",
  "mergeBaseOid": "2aec49acc9bf0e147ea91e7319472f7daec83154",
  "lastPushDate": "2022-08-10T14:13:22.000Z",
  "lastActivityDate": "2022-08-19T13:14:14.000Z",
  "mergeOfferDate": "2022-08-19T13:08:38.000Z",
  "mergeRequestDate": "2022-08-19T13:14:14.000Z",
  "mergeRequestUser": "RC-14",
  "hasMergeConflict": false,
  "isFirstContribution": true,
  "tooManyFiles": false,
  "hugeChange": false,
  "popularityLevel": "Popular",
  "pkgInfo": [
    {
      "name": "chrome",
      "kind": "edit",
      "files": [
        {
          "path": "types/chrome/index.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/chrome/test/index.ts",
          "kind": "test"
        }
      ],
      "owners": [
        "matthewkimber",
        "otiai10",
        "sreimer15",
        "MatCarlson",
        "ekinsol",
        "tregagnon",
        "echoabstract",
        "spasma",
        "bdbai",
        "pokutuna",
        "JasonXian",
        "usertim",
        "idan315",
        "nicolas377",
        "idosal"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Popular"
    }
  ],
  "reviews": [
    {
      "type": "approved",
      "reviewer": "nicolas377",
      "date": "2022-08-19T13:07:54.000Z",
      "isMaintainer": false
    }
  ],
  "mainBotCommentID": 1210788754,
  "ciResult": "pass"
}

typescript-bot avatar Aug 10 '22 14:08 typescript-bot

🔔 @matthewkimber @otiai10 @sreimer15 @MatCarlson @ekinsol @tregagnon @echoabstract @spasma @bdbai @pokutuna @JasonXian @usertim @idan315 @nicolas377 @idosal — 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.

typescript-bot avatar Aug 10 '22 14:08 typescript-bot

This PR fixes the issue mentioned here: https://github.com/DefinitelyTyped/DefinitelyTyped/discussions/60954

RC-14 avatar Aug 10 '22 14:08 RC-14

Ready to merge

RC-14 avatar Aug 19 '22 13:08 RC-14

@nicolas377 I completely agree with @mdmower. MV2 extensions are still the overwhelming majority of extensions in the Chrome Web Store, and will be maintained for at least another 8-9 months. We must maintain backwards compatibility, or most people will stop using this library.

idosal avatar Oct 10 '22 07:10 idosal

IMHO, TS is kinda about best practices, and its definitely best practice to use MV3. But that's pretty irrelevant here, because if we did support MV2, we couldn't support MV3 at the same time without a lot of work that'll just be irrelevant in a few months. Also, I'm pretty sure that MV2 hasn't had api changes since we stopped supporting it in these types, so if needed, you can stay on the older types. I'm not really sure what to do to fix this, so feel free to suggest stuff in discussions!

nicolas377 avatar Oct 17 '22 19:10 nicolas377

This seems similar to other breaking changes in this repo and DefinitelyTyped has a recommendation: https://github.com/DefinitelyTyped/DefinitelyTyped#if-a-library-is-updated-to-a-new-major-version-with-breaking-changes-how-should-i-update-its-type-declaration-package

It's a bit strange since this package doesn't map it's releases to chrome versions but I feel that treating manifest version as the major version makes sense.

In that case should we create a v2 directory for hosting the MV2 compatible types and allow the top level to continue targeting MV3?

One problem with telling devs to target the proper version manually is that it's hard to know which version that is without each dev researching the commit history. Also, there have been corrections to older APIs before though maybe that hasn't happened recently.

Given MV2 is still under active support until 2024 it doesn't seem like a waste of time (similar to @types/node which still supports v14 and v16)

melink14 avatar Nov 18 '22 06:11 melink14

I agree that it would be useful to separate MV2 and MV3 types. Currently I never truly know if a type that I've never used before is up to date and have to check before using it which defeats (part) of the purpose of types. I'd prefer splitting the types up and having (more) missing types over having to worry about possibly incorrect types that might or might not work.

As it is right now I just can't be bothered to add types if I don't need them and even then it's not exactly fun to check what's outdated and has to be changed/deleted and then actually do that.

@melink14 If you want to start a discussion about it and the maintainers agree to split up the types I'd gladly contribute some MV3 types.

RC-14 avatar Nov 21 '22 17:11 RC-14