@weswigham Thank you for submitting this PR!

This is a live comment which I will keep updated.

1 package in this PR

• marked — on npm, on unpkg
  • owner-approval: @UziTech
  • Config files to check:
    • marked/OTHER_FILES.txt: not the expected form
    • marked/index.d.mts: edited
    • marked/package.json: not the expected form (check: exports)

Code Reviews

Because this PR edits the configuration file, it can be merged once it's reviewed by a DT maintainer.

You can test the changes of this PR in the Playground.

Status

• ✅ No merge conflicts
• ✅ Continuous integration tests have passed
• 🕐 A DT maintainer needs to approve changes which affect module config files

Once every item on this list is checked, I'll ask you for permission to merge and publish the changes.

{
  "type": "info",
  "now": "-",
  "pr_number": 57236,
  "author": "weswigham",
  "headCommitOid": "5f2eadf20d68a1acc3a2d39ea499862a8d5f3c46",
  "lastPushDate": "2021-11-17T21:41:35.000Z",
  "lastActivityDate": "2021-11-17T22:00:03.000Z",
  "hasMergeConflict": false,
  "isFirstContribution": false,
  "tooManyFiles": false,
  "popularityLevel": "Popular",
  "pkgInfo": [
    {
      "name": "marked",
      "kind": "edit",
      "files": [
        {
          "path": "types/marked/OTHER_FILES.txt",
          "kind": "package-meta",
          "suspect": "not [the expected form](https://github.com/DefinitelyTyped/DefinitelyTyped#user-content-other_filestxt)"
        },
        {
          "path": "types/marked/index.d.mts",
          "kind": "package-meta",
          "suspect": "edited"
        },
        {
          "path": "types/marked/package.json",
          "kind": "package-meta",
          "suspect": "not [the expected form](https://github.com/DefinitelyTyped/DefinitelyTyped#user-content-packagejson) (check: `exports`)"
        }
      ],
      "owners": [
        "worr",
        "BendingBender",
        "CrossR",
        "mwickett",
        "htkzhtm",
        "ezracelli",
        "scandinave",
        "sarunint",
        "UziTech",
        "Toliak"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Popular"
    }
  ],
  "reviews": [
    {
      "type": "approved",
      "reviewer": "UziTech",
      "date": "2021-11-17T22:00:03.000Z",
      "isMaintainer": false
    }
  ],
  "mainBotCommentID": 972087140,
  "ciResult": "pass"
}

🔔 @worr @BendingBender @CrossR @mwickett @htkzhtm @ezracelli @scandinave @sarunint @UziTech @Toliak — 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.

just noticed this when updating one of our repos. @weswigham would you mind explaining what the new mts file's purpose is?

the existing index.d.ts already has named exports, which are accurate under CJS and ESM (since there's no node-specific nonsense like export assignments).

out of curiosity more than anything, what does it solve that was presumably broken before? i.e. couldn't you have just pointed both the cjs and esm type exports to the same file since they really are the same as far as the type system is concerned.

It allows TS to detect the correct format data for the import, and thus prevent getting a phantom default import member when importing in esm.

since there is no export assignment (CJS) or default export, the existing types were correct for both CJS and ESM consumption, as far as i can see.

tsc would have loaded types from @types/marked which would've been index.d.ts - type definitions which are correct regardless of your module system.

where would we get a default export? maybe im just missing some special behaviour tsc has, but it looks like we should only need two type entry points if the types differ between CJS and ESM (which they don't here).

where would we get a default export?

An esm file importing a cjs file module has the whole cjs module available as a default - this is node's behavior. An import of an esm file, meanwhile, does not.

but here we're only concerned with the type system. so does typescript apply some special behaviour where it fabricates a default export?

before this PR, importing this in ESM and CJS worked as expected. so the situation where it wouldn't work was what?

sorry for the many questions, just trying to get my head around why this change was needed in case it pops up elsewhere and to be sure it isn't redundant.

It's not so much TS "giving a module a default" as TS building the module's externally visible exports in a way closer to node itself?

For context, when an esm file in node imports a cjs file, node syntactically finds export names in the cjs file, hoists those names to be visible as named imports, and then makes the module itself available as the default import (so, for example, export assigned modules are available). If the cjs module actually has an export named default, which is often the case, such a member becomes available as the default member of the default import. Eg, if you wrote import * as NS from "mod", you'd have to write NS.default.default to get the actual cjs module's 'default'.

yup i understand all of that now

in the package exports, you could've technically pointed both at the same d.ts though, right? since there's no difference, both export exactly the same types as far as tsc is concerned. and in that case, why did we need the exports map at all?

i guess the bit im missing is: how does this change help with what you explained. typescript will see the exact same exports before and after this change, so is it doing something special just because its a d.mts file?

No, you can't point both at the same file because the format is determined by the file, not the export map condition, and a file only has one format. A import condition is free to point at a cjs file if you'd like.

so is it doing something special just because its a d.mts file?

Yes.

ok i kinda see now.

i had a play around and i see, if you import from a cjs file (resolving to cjs/cts/d.cts), you have a default export always available. in all other cases, there isn't one unless we literally exported a default (esm).

so i guess the default must be commonjs since we haven't given it a cts extension.