[markdown-it] Move to ES modules by runarberg · Pull Request #68924 · DefinitelyTyped/DefinitelyTyped

runarberg · 2024-03-07T02:53:11Z

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://github.com/markdown-it/markdown-it/blob/master/CHANGELOG.md#1400---2023-12-08
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.

typescript-bot · 2024-03-07T02:53:51Z

@runarberg 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.

6 packages in this PR

markdown-it-container — on npm, on unpkg
markdown-it-emoji — on npm, on unpkg
markdown-it-external-links — on npm, on unpkg
markdown-it-link-attributes — on npm, on unpkg
markdown-it-plantuml — on npm, on unpkg
markdown-it — on npm, on unpkg

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
✅ A DT maintainer needs to approve changes which affect more than one package

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": 68924,
  "author": "runarberg",
  "headCommitOid": "a0a8f417f48ea59bb0af2cf12d967d646cc74d8b",
  "mergeBaseOid": "5796940dfa7daf1b8a701dbe24df678bd8ab8e81",
  "lastPushDate": "2024-03-07T02:53:11.000Z",
  "lastActivityDate": "2024-04-05T21:22:59.000Z",
  "mergeOfferDate": "2024-04-05T19:54:53.000Z",
  "mergeRequestDate": "2024-04-05T21:22:59.000Z",
  "mergeRequestUser": "runarberg",
  "hasMergeConflict": false,
  "isFirstContribution": true,
  "tooManyFiles": false,
  "hugeChange": false,
  "popularityLevel": "Critical",
  "pkgInfo": [
    {
      "name": "markdown-it-container",
      "kind": "edit",
      "files": [
        {
          "path": "types/markdown-it-container/index.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/markdown-it-container/markdown-it-container-tests.ts",
          "kind": "test"
        }
      ],
      "owners": [
        "hronex"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Well-liked by everyone"
    },
    {
      "name": "markdown-it-emoji",
      "kind": "edit",
      "files": [
        {
          "path": "types/markdown-it-emoji/index.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/markdown-it-emoji/test/markdown-it-emoji-global.ts",
          "kind": "test"
        }
      ],
      "owners": [
        "Shinigami92",
        "peterblazejewicz"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Well-liked by everyone"
    },
    {
      "name": "markdown-it-external-links",
      "kind": "edit",
      "files": [
        {
          "path": "types/markdown-it-external-links/index.d.ts",
          "kind": "definition"
        }
      ],
      "owners": [
        "grawl"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Well-liked by everyone"
    },
    {
      "name": "markdown-it-link-attributes",
      "kind": "edit",
      "files": [
        {
          "path": "types/markdown-it-link-attributes/index.d.ts",
          "kind": "definition"
        }
      ],
      "owners": [
        "cowpewter",
        "peterblazejewicz"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Well-liked by everyone"
    },
    {
      "name": "markdown-it-plantuml",
      "kind": "edit",
      "files": [
        {
          "path": "types/markdown-it-plantuml/index.d.ts",
          "kind": "definition"
        }
      ],
      "owners": [
        "gmunguia"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Well-liked by everyone"
    },
    {
      "name": "markdown-it",
      "kind": "edit",
      "files": [
        {
          "path": "types/markdown-it/dist/index.cjs.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/markdown-it/dist/markdown-it.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/markdown-it/dist/markdown-it.min.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/markdown-it/index.d.mts",
          "kind": "definition"
        },
        {
          "path": "types/markdown-it/index.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/markdown-it/lib/common/entities.d.mts",
          "kind": "definition"
        },
        {
          "path": "types/markdown-it/lib/common/html_blocks.d.mts",
          "kind": "definition"
        },
        {
          "path": "types/markdown-it/lib/common/html_re.d.mts",
          "kind": "definition"
        },
        {
          "path": "types/markdown-it/lib/common/utils.d.mts",
          "kind": "definition"
        },
        {
          "path": "types/markdown-it/lib/common/utils.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/markdown-it/lib/helpers/index.d.mts",
          "kind": "definition"
        },
        {
          "path": "types/markdown-it/lib/helpers/index.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/markdown-it/lib/helpers/parse_link_destination.d.mts",
          "kind": "definition"
        },
        {
          "path": "types/markdown-it/lib/helpers/parse_link_label.d.mts",
          "kind": "definition"
        },
        {
          "path": "types/markdown-it/lib/helpers/parse_link_title.d.mts",
          "kind": "definition"
        },
        {
          "path": "types/markdown-it/lib/index.d.mts",
          "kind": "definition"
        },
        {
          "path": "types/markdown-it/lib/parser_block.d.mts",
          "kind": "definition"
        },
        {
          "path": "types/markdown-it/lib/parser_core.d.mts",
          "kind": "definition"
        },
        {
          "path": "types/markdown-it/lib/parser_core.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/markdown-it/lib/parser_inline.d.mts",
          "kind": "definition"
        },
        {
          "path": "types/markdown-it/lib/renderer.d.mts",
          "kind": "definition"
        },
        {
          "path": "types/markdown-it/lib/ruler.d.mts",
          "kind": "definition"
        },
        {
          "path": "types/markdown-it/lib/rules_block/state_block.d.mts",
          "kind": "definition"
        },
        {
          "path": "types/markdown-it/lib/rules_core/state_core.d.mts",
          "kind": "definition"
        },
        {
          "path": "types/markdown-it/lib/rules_inline/state_inline.d.mts",
          "kind": "definition"
        },
        {
          "path": "types/markdown-it/lib/token.d.mts",
          "kind": "definition"
        },
        {
          "path": "types/markdown-it/markdown-it-tests.ts",
          "kind": "test"
        },
        {
          "path": "types/markdown-it/package.json",
          "kind": "package-meta-ok"
        },
        {
          "path": "types/markdown-it/test/common/utils.mts",
          "kind": "test"
        },
        {
          "path": "types/markdown-it/test/helpers/index.mts",
          "kind": "test"
        },
        {
          "path": "types/markdown-it/test/helpers/index.ts",
          "kind": "test"
        },
        {
          "path": "types/markdown-it/test/index.mts",
          "kind": "test"
        },
        {
          "path": "types/markdown-it/test/parser_block.mts",
          "kind": "test"
        },
        {
          "path": "types/markdown-it/test/parser_core.mts",
          "kind": "test"
        },
        {
          "path": "types/markdown-it/test/parser_core.ts",
          "kind": "test"
        },
        {
          "path": "types/markdown-it/test/parser_inline.mts",
          "kind": "test"
        },
        {
          "path": "types/markdown-it/test/parser_inline.ts",
          "kind": "test"
        },
        {
          "path": "types/markdown-it/test/renderer.mts",
          "kind": "test"
        },
        {
          "path": "types/markdown-it/test/ruler.mts",
          "kind": "test"
        },
        {
          "path": "types/markdown-it/test/rules_block/state_block.mts",
          "kind": "test"
        },
        {
          "path": "types/markdown-it/test/rules_core/state_core.mts",
          "kind": "test"
        },
        {
          "path": "types/markdown-it/test/rules_inline/state_inline.mts",
          "kind": "test"
        },
        {
          "path": "types/markdown-it/test/token.mts",
          "kind": "test"
        },
        {
          "path": "types/markdown-it/tsconfig.json",
          "kind": "package-meta-ok"
        }
      ],
      "owners": [
        "plantain-00",
        "rapropos",
        "duduluu",
        "peterblazejewicz"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Critical"
    }
  ],
  "reviews": [
    {
      "type": "approved",
      "reviewer": "jakebailey",
      "date": "2024-04-05T19:54:06.000Z",
      "isMaintainer": true
    },
    {
      "type": "stale",
      "reviewer": "RyanCavanaugh",
      "date": "2024-04-04T20:28:37.000Z",
      "abbrOid": "34f3509"
    },
    {
      "type": "stale",
      "reviewer": "weswigham",
      "date": "2024-03-11T22:52:35.000Z",
      "abbrOid": "a8abc64"
    }
  ],
  "mainBotCommentID": 1982243797,
  "ciResult": "pass"
}

typescript-bot · 2024-03-07T02:53:53Z

🔔 @Hronex @Shinigami92 @peterblazejewicz @Grawl @cowpewter @gmunguia @plantain-00 @rapropos @duduluu — 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 · 2024-03-07T02:56:50Z

@runarberg 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.

runarberg · 2024-03-07T02:59:10Z

This is my first contribution, and I’m not sure if I’m doing things correctly here.

I couldn’t get the tests to pass, unless I duplicated the code for the commonjs modules. Version 14 of Markdown-It builds the dist/ directory, and only exports a single file there. So I’m not sure if you can actually import from /lib (or subdirectories inside /dist) any more using commonjs any longer.

typescript-bot · 2024-03-07T05:09:48Z

@runarberg 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.

DangerBotOSS · 2024-03-07T05:20:02Z

Formatting

The following files are not formatted:

types/markdown-it/test/index.mts
types/markdown-it/lib/index.d.mts
types/markdown-it/lib/parser_inline.d.mts
types/markdown-it/test/helpers/index.mts
types/markdown-it/lib/helpers/index.d.mts

as well as these 1 other files...

types/markdown-it/dist/markdown-it.d.ts

Consider running pnpm dprint fmt on these files to make review easier.

Generated by 🚫 dangerJS against a0a8f41

weswigham

I couldn’t get the tests to pass, unless I duplicated the code for the commonjs modules. Version 14 of Markdown-It builds the dist/ directory, and only exports a single file there. So I’m not sure if you can actually import from /lib (or subdirectories inside /dist) any more using commonjs any longer.

Looks like the package ships a single cjs file in dist, but exclusively esm in lib, and actually does expose both in its exports map (imo, a sloppy and very breaky migration to esm!). You'll want to duplicate that structure here - dist should just have a single markdown-it.d.ts in it, with types reexported in index.cjs.d.ts and markdown-it.min.d.ts. lib also needs all its files renamed to .d.mts to match all the .mjs files now in lib (which you've done). You'll also need to add an export map that roughly matches their published one (which you've also done).

This will break every other package doing a deep import of markdown-it - but so, too, did the source package. If the dependencies actually work with the new markdown-it, they'll have to be updated to use a shallow import or mode-appropriate import, otherwise be fixed to an old version of the markdown-it types.

typescript-bot · 2024-03-11T22:53:22Z

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

runarberg · 2024-03-12T02:34:52Z

@weswigham You can still access the exported symbols from the default export, albeit sometimes via a very roundabout way:

const MarkdownIt = require("markdown-it");

// import Token from "markdown-it/lib/token.mjs";
const Token = MarkdownIt().core.State.prototype.Token;

I’m guessing downstream libraries will have to go an equally roundabout way of getting the type symbols:

import MarkdownIt = require("markdown-it");
type Token = InstanceType<MarkdownIt["core"]["State"]>["Token"];

weswigham · 2024-03-12T02:42:47Z

I’m guessing downstream libraries will have to go an equally roundabout way of getting the type symbols

Yes and no - for actual value shapes, yes - if that's where the values are at runtime, that's where they gotta be. For type-only things like interfaces? You can probably get away with exposing them under a namespace merged with the top level call (for cjs), or directly at the top level (for esm). There's a long history of types being more easily accessible than the existing values that use them.

typescript-bot · 2024-03-14T01:43:59Z

@runarberg 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.

typescript-bot · 2024-04-04T06:45:22Z

@runarberg I haven't seen any activity on this PR in more than three weeks, and it still has problems that prevent it from being merged. The PR will be closed on Apr 11th (in a week) if the issues aren't addressed.

typescript-bot · 2024-04-04T19:31:05Z

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, @runarberg.

(Ping @Hronex, @Shinigami92, @peterblazejewicz, @Grawl, @cowpewter, @gmunguia, @plantain-00, @rapropos, @duduluu.)

typescript-bot · 2024-04-04T19:34:19Z

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

runarberg · 2024-04-05T00:07:44Z

There is a fair bit of duplicated code here, I’m wondering if it were better to import the typedefs from dist/markdown-it.d.ts to the lib/*.mts files.

I also worry that the tests are separated between the lib/*.mts typedefs and dist/markdown-it.d.ts typedefs. I didn’t really add tests that cover the stuff I moved to the main namespace from the various modules in the commonjs portion.

jakebailey · 2024-04-05T00:09:23Z

You can totally do that; if it compiles and node16 is enabled, that should work.

typescript-bot · 2024-04-05T19:36:28Z

@RyanCavanaugh, @weswigham 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?

runarberg · 2024-04-05T19:37:33Z

@jakebailey I tried to reference the type definitions in dist/markdown-it.d.ts from the lib/*.mts files, but couldn’t cleanly separate the namespace imports from the class/value imports (unsurprising really, I never use namespaces in my own code and don’t really know what I’m doing 😉). So I’m gonna leave the duplicated typedefs as is.

However I amended the PR with the tests copy-pasted from /tests/*.mts files over to the main markdown-it-tests.ts (and discovered an error and a type-o doing so), so I feel a little more confident in the correctness now.

jakebailey

alright, so long as you are okay with someone potentially accidentally changing one declaration file but not another!

typescript-bot · 2024-04-05T19:54:53Z

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

runarberg · 2024-04-05T21:22:59Z

Ready to merge

runarberg requested a review from peterblazejewicz as a code owner March 7, 2024 02:53

typescript-bot added the Critical package label Mar 7, 2024

typescript-bot added the The CI failed When GH Actions fails label Mar 7, 2024

typescript-bot added Edits multiple packages The CI failed When GH Actions fails and removed The CI failed When GH Actions fails labels Mar 7, 2024

typescript-bot removed the The CI failed When GH Actions fails label Mar 7, 2024

weswigham requested changes Mar 11, 2024

View reviewed changes

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

typescript-bot added The CI failed When GH Actions fails and removed Revision needed This PR needs code changes before it can be merged. labels Mar 14, 2024

typescript-bot added the Abandoned This PR had no activity for a long time, and is considered abandoned label Apr 4, 2024

typescript-bot added Unreviewed No one showed up to review this PR, so it'll be reviewed by a DT maintainer. and removed Abandoned This PR had no activity for a long time, and is considered abandoned The CI failed When GH Actions fails labels Apr 4, 2024

RyanCavanaugh approved these changes Apr 4, 2024

View reviewed changes

typescript-bot added the Maintainer Approved label Apr 4, 2024

typescript-bot added Self Merge This PR can now be self-merged by the PR author or an owner and removed Unreviewed No one showed up to review this PR, so it'll be reviewed by a DT maintainer. labels Apr 4, 2024

[markdown-it] Move to ES modules

a0a8f41

typescript-bot added Unreviewed No one showed up to review this PR, so it'll be reviewed by a DT maintainer. and removed Self Merge This PR can now be self-merged by the PR author or an owner Maintainer Approved labels Apr 5, 2024

jakebailey approved these changes Apr 5, 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 and removed Unreviewed No one showed up to review this PR, so it'll be reviewed by a DT maintainer. labels Apr 5, 2024

typescript-bot merged commit a208337 into DefinitelyTyped:master Apr 5, 2024

brc-dd mentioned this pull request Apr 15, 2024

[markdown-it,mdurl,linkify-it,markdown-it-emoji] update type definitions #69355

Merged

8 tasks

Conversation

runarberg commented Mar 7, 2024

Uh oh!

typescript-bot commented Mar 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

6 packages in this PR

Code Reviews

Status

Uh oh!

typescript-bot commented Mar 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

typescript-bot commented Mar 7, 2024

Uh oh!

runarberg commented Mar 7, 2024

Uh oh!

typescript-bot commented Mar 7, 2024

Uh oh!

DangerBotOSS commented Mar 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Formatting

Uh oh!

weswigham left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

typescript-bot commented Mar 11, 2024

Uh oh!

runarberg commented Mar 12, 2024

Uh oh!

weswigham commented Mar 12, 2024

Uh oh!

typescript-bot commented Mar 14, 2024

Uh oh!

typescript-bot commented Apr 4, 2024

Uh oh!

typescript-bot commented Apr 4, 2024

Uh oh!

typescript-bot commented Apr 4, 2024

Uh oh!

runarberg commented Apr 5, 2024

Uh oh!

jakebailey commented Apr 5, 2024

Uh oh!

typescript-bot commented Apr 5, 2024

Uh oh!

runarberg commented Apr 5, 2024

Uh oh!

jakebailey left a comment

Choose a reason for hiding this comment

Uh oh!

typescript-bot commented Apr 5, 2024

Uh oh!

runarberg commented Apr 5, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

typescript-bot commented Mar 7, 2024 •

edited

Loading

typescript-bot commented Mar 7, 2024 •

edited

Loading

DangerBotOSS commented Mar 7, 2024 •

edited

Loading

weswigham left a comment •

edited

Loading