feat: support punycode js file imports

[hrueger]

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: <>
• 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.
• Other: punycode is also a builtin node module. To explicitely load the npm package instead, people have been adding a / suffix to the import. However, this breaks some bundlers and esm (example), which means the import needs to be punycode/punycode.js (example). Currently, typescript complains about that, but this PR fixes that.

[typescript-bot]

@hrueger Thank you for submitting this PR!

This is a live comment which I will keep updated.

1 package in this PR

• punycode — 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 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": 68273,
  "author": "hrueger",
  "headCommitOid": "26cfd1d5d06beaef2e59c45212e43c545ce69a2b",
  "mergeBaseOid": "c7c3b75eeed5fd5edc16914acb1e011769f9a643",
  "lastPushDate": "2024-01-21T20:13:11.000Z",
  "lastActivityDate": "2024-02-12T17:20:18.000Z",
  "mergeOfferDate": "2024-02-12T17:19:17.000Z",
  "mergeRequestDate": "2024-02-12T17:20:18.000Z",
  "mergeRequestUser": "hrueger",
  "hasMergeConflict": false,
  "isFirstContribution": false,
  "tooManyFiles": false,
  "hugeChange": false,
  "popularityLevel": "Popular",
  "pkgInfo": [
    {
      "name": "punycode",
      "kind": "edit",
      "files": [
        {
          "path": "types/punycode/punycode-tests.ts",
          "kind": "test"
        },
        {
          "path": "types/punycode/punycode.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/punycode/tsconfig.json",
          "kind": "package-meta-ok"
        }
      ],
      "owners": [
        "yavanosta"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Popular"
    }
  ],
  "reviews": [
    {
      "type": "approved",
      "reviewer": "jakebailey",
      "date": "2024-02-12T17:18:34.000Z",
      "isMaintainer": true
    }
  ],
  "mainBotCommentID": 1902748520,
  "ciResult": "pass"
}

[typescript-bot]

🔔 @Yavanosta — 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]

Re-ping @Yavanosta:

This PR has been out for over a week, yet I haven't seen any reviews.

Could someone please give it some attention? Thanks!

[typescript-bot]

It has been more than two weeks and this PR still has no reviews.

I'll bump it to the DT maintainer queue. Thank you for your patience, @hrueger.

(Ping @Yavanosta.)

[jakebailey]

This should just be punycode.d.ts, because d.ts files type .js files. But, it's not clear to me why this is needed.

[jakebailey]

I see now that this shadows a node module, so that makes slightly more sense. Probably this should be export = require("punycode") to match module shape.

[hrueger]

Hi @jakebailey,
thanks for reviewing.

When I rename the file from punycode.js.d.ts to punycode.d.ts, I get the following error message in the test: Could not find a declaration file for module 'punycode/punycode.js'. :

If I use export = require("punycode"), I get this:

I added the reason for this change in the pr description at the bottom, maybe it was not clear enough:
As you said, punycode is also a builtin node module. To explicitely load the npm package instead, people have been adding a / suffix to the import. However, this breaks some bundlers and esm (salesforce/tough-cookie#256), which means the import needs to be punycode/punycode.js (example). Currently, typescript complains about that, but this PR fixes that.

[jakebailey]

I don't have the same problem with this diff:

diff --git a/types/punycode/punycode.d.ts b/types/punycode/punycode.d.ts
new file mode 100644
index 0000000000..fd4ea774ee
--- /dev/null
+++ b/types/punycode/punycode.d.ts
@@ -0,0 +1,2 @@
+import punycode = require(".");
+export = punycode;
diff --git a/types/punycode/punycode.js.d.ts b/types/punycode/punycode.js.d.ts
deleted file mode 100644
index ef8f0464f7..0000000000
--- a/types/punycode/punycode.js.d.ts
+++ /dev/null
@@ -1 +0,0 @@
-export * from "punycode";
diff --git a/types/punycode/tsconfig.json b/types/punycode/tsconfig.json
index 5a7dfe15b2..674baae5b8 100644
--- a/types/punycode/tsconfig.json
+++ b/types/punycode/tsconfig.json
@@ -14,7 +14,7 @@
     },
     "files": [
         "index.d.ts",
-        "punycode.js.d.ts",
+        "punycode.d.ts",
         "punycode-tests.ts"
     ]
 }

I made a mistake on export = require("punycode"); sorry about that.

[hrueger]

Thanks for the clarification @jakebailey . Could you please take another look?

[typescript-bot]

@hrueger 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]

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

[jakebailey]

LGTM, but I think in the future we're going to attempt to make index.d.ts not a required file for DT packages such that this code could just be stored in punycode.d.ts only and be more accurate.

[typescript-bot]

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

(@Yavanosta: you can do this too.)

[hrueger]

Thanks @jakebailey !

Ready to merge