Support ES Module output in npm package. #862

[paulirwin]

Adds ES Module (ESM) output in the Rollup config (in addition to the existing UMD output), and adds a helpful npm run build script. Fixes #862

Per the CONTRIBUTING.md file, I am not including the ESM build here, but I can easily add that if you would like.

Example Angular component using the ESM build (add to package.json as a file-path dependency) which does not have the warning about optimization bailouts:

import { Component, OnInit } from '@angular/core';
import MarkdownIt from 'markdown-it';

@Component({
  selector: 'app-root',
  templateUrl: './app.component.html',
  styleUrls: ['./app.component.scss']
})
export class AppComponent implements OnInit {
  title = 'markdown-it-ng-demo';
  text: string = "";

  ngOnInit(): void {
    const md = new MarkdownIt();
    console.log(md);
    this.text = md.render("# Hi!");
    console.log(this.text);
  }
}

Template HTML:

<div [innerHTML]="text"></div>

[thiagodp]

Hello @paulirwin,
I suggest using microbundle for that. It provides different bundle formats with a minimum effort.

[paulirwin]

@thiagodp I appreciate the input, but this project already uses Rollup, so changing that is outside the scope of this PR.

[thiagodp]

@paulirwin Ok. JFI, Microbundle is based on Rollup, and you could continue using the existing plug-ins.

[tdekoning]

@paulirwin any update on this? Would be a great addition to the library

[paulirwin]

@tdekoning I also would like to see this PR merged 😉

[pauleustice]

I'd also really like to see this merged please 🙏

[andymac4182]

Any update on this? It would help resolve some issues I am having.

[paulirwin]

Tagging the two people listed in the markdown-it org: @puzrin @rlidwka

It appears the community is longing to get this fix merged into the project. I'd be happy to address any feedback with it if there's anything preventing this from getting merged. Let me know how I can be of any assistance. Thanks!

[vritant24]

Would very much like to see this PR merged. Happy to help take this through if help is needed.

[JannieT]

As a workaround, I'm using this: https://github.com/esm-bundle/markdown-it

[paulirwin]

I am generally a patient person when it comes to things like this, but this PR has been open for over a year and is fairly simple. It has been over two months since I pinged the owners of this repo, with no response. There have been no merged PRs since August of 2022 as far as I can tell. Seriously asking, is this project dead?

[hutianyu2006]

How could it not being merged for SOOOOO LONG? It will be uncomfortable to see a mixed usage of esm & native javascriprt in any node.js project, and it will also cause confusion for IDEs.

[treeder]

Marked has ESM builds, FYI: https://www.npmjs.com/package/marked

or in browser:

import { marked } from "https://cdn.jsdelivr.net/npm/marked/lib/marked.esm.js"

[hutianyu2006]

OMG I simply didn't do too much searching, THANK YOU A LOT!!!

…

---Original--- From: "Travis ***@***.***> Date: Fri, Jun 30, 2023 02:39 AM To: ***@***.***>; Cc: ***@***.******@***.***>; Subject: Re: [markdown-it/markdown-it] Support ES Module output in npmpackage. #862 (PR #863) Mark has ESM builds, FYI: https://www.npmjs.com/package/marked or in browser: import { marked } from "https://cdn.jsdelivr.net/npm/marked/lib/marked.esm.js" — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you commented.Message ID: ***@***.***>

[paulirwin]

@treeder Unfortunately this is a transitive dependency for me so that isn't an option for all cases 😦

[marijnh]

In 2023, we'll also want an exports field in package.json, something like ...

  "exports": {
    "require": "./index.js",
    "import": "./dist/esm/markdown-it.mjs"
  },

That makes sure resolution that doesn't use the non-standard "module" field (such as node's own) can still find the ESM code.

(Just verified that this PR still applies cleanly, but node will load the CJS code when you import it. With the above change to package.json, it loads the ESM code, and everything seems to work.)

Further edit: Actually, adding this will forbid importing modules directly from the package by path, which is maybe a style you intend to support, given that there are separate minified files. Which modules are users expected to want to load directly?

[andymac4182]

This would be great to have supported!

[adrian-branescu]

@paulirwin @marijnh
With the following change:

"exports": {
    ".": {
      "import": "./dist/esm/markdown-it.mjs",
      "require": "./index.js"
    },
    "./*": {
      "require": "./*",
      "import": "./*"
    }
},

will be this PR eventually merged?

[paulirwin]

Re: @marijnh (I am forever indebted to you for your excellent work on ProseMirror, btw. Thank you for that library!)

In 2023, we'll also want an exports field in package.json, something like ...

  "exports": {
    "require": "./index.js",
    "import": "./dist/esm/markdown-it.mjs"
  },

That makes sure resolution that doesn't use the non-standard "module" field (such as node's own) can still find the ESM code.

This is good to know, thanks! I don't spend a lot of time in Node directly and only use this package via Angular. Should "type": "module" be added as well, or would that break "legacy" users? Do we still need to support CommonJS now that it's almost 2024 and Node v20 is LTS?

(Just verified that this PR still applies cleanly, but node will load the CJS code when you import it. With the above change to package.json, it loads the ESM code, and everything seems to work.)

Further edit: Actually, adding this will forbid importing modules directly from the package by path, which is maybe a style you intend to support, given that there are separate minified files. Which modules are users expected to want to load directly?

I don't have any particular intentions if this was addressed to me, other than ensuring that optimization in Angular builds works. I am a little out-of-the-loop of current Node package conventions and standards. I know at one point in the past, it was fairly common to include minified builds with your package like markdown-it has done prior to my PR. Is that no longer the case? If so, that would certainly simplify the Rollup config and build. I'm eager to learn from the community here on any ways I can improve this PR.

Re: @adrian-branescu

@paulirwin @marijnh With the following change:

"exports": {
    ".": {
      "import": "./dist/esm/markdown-it.mjs",
      "require": "./index.js"
    },
    "./*": {
      "require": "./*",
      "import": "./*"
    }
},

What does the second export with the * accomplish, and why is that needed here? Looking to learn as I am unfamiliar with that.

will be this PR eventually merged?

I am encouraged to see recent activity in September of this year on this project. It would be great if this could get merged at some point. Obviously as the PR author I would love to see it merged, but alas I am not a contributor to this repo. It sounds like I do need to brush the dust off of it before it is merged, though, with the addition of exports.

[marijnh]

Should "type": "module" be added as well, or would that break "legacy" users?

That should be fine, and help some tools that still use the old convention.

What does the second export with the * accomplish, and why is that needed here? Looking to learn as I am unfamiliar with that.

Once you have an exports field, only the files mentioned in it can be loaded from the package. So, since this package has more than just a top-level entry point, adding these wildcards makes those sub-paths accessible for direct require.

I would hope updating this PR and prodding a bit more will lead to a new release.

[paulirwin]

@marijnh @adrian-branescu Thank you! I have made those changes to this PR. Quick testing of a new Node package that references it by file path seems to work fine at least for constructing a MarkdownIt. If either of you can help validate that I did this correctly and sufficiently, that would be appreciated.

[adrian-branescu]

@paulirwin @marijnh
I would add to the rollup.config.js file the external field, e.g.

external: [
  /node_modules/
]

in order to prevent including in the bundle the external dependencies (along with the whole graph of transitive dependencies) from package.json:

"dependencies": {
    "argparse": "^2.0.1",
    "entities": "~3.0.1",
    "linkify-it": "^3.0.1",
    "mdurl": "^1.0.1",
    "uc.micro": "^1.0.5"
},

In this way, one can use this NPM package in Node.js and Web apps with a bundler like Webpack, Rollup, Vite etc without having those dependencies included multiple times in the final app bundle.

[marijnh]

Should "type": "module" be added as well, or would that break "legacy" users?

That should be fine, and help some tools that still use the old convention.

Correction! I thought you were talking about the "module": "dist/esm/markdown-it.mjs" line. "type": "module" should definitely NOT be added here. @paulirwin

[paulirwin]

@paulirwin @marijnh I would add to the rollup.config.js file the external field, e.g.

external: [
  /node_modules/
]

in order to prevent including in the bundle the external dependencies (along with the whole graph of transitive dependencies) from package.json:

"dependencies": {
    "argparse": "^2.0.1",
    "entities": "~3.0.1",
    "linkify-it": "^3.0.1",
    "mdurl": "^1.0.1",
    "uc.micro": "^1.0.5"
},

In this way, one can use this NPM package in Node.js and Web apps with a bundler like Webpack, Rollup, Vite etc without having those dependencies included multiple times in the final app bundle.

If I'm understanding this correctly, this would be an existing problem with the current rollup CJS output, is that correct? If so it seems like that warrants its own PR and could be merged independently.

[marijnh]

Thanks. With that change this patch works for me (I tried: importing as ESM, requiring as CommonJS, requiring a specific dist/ file.)

[adrian-branescu]

@paulirwin @marijnh I would add to the rollup.config.js file the external field, e.g.

external: [
  /node_modules/
]

in order to prevent including in the bundle the external dependencies (along with the whole graph of transitive dependencies) from package.json:

"dependencies": {
    "argparse": "^2.0.1",
    "entities": "~3.0.1",
    "linkify-it": "^3.0.1",
    "mdurl": "^1.0.1",
    "uc.micro": "^1.0.5"
},

In this way, one can use this NPM package in Node.js and Web apps with a bundler like Webpack, Rollup, Vite etc without having those dependencies included multiple times in the final app bundle.

If I'm understanding this correctly, this would be an existing problem with the current rollup CJS output, is that correct? If so it seems like that warrants its own PR and could be merged independently.

Yes, this problem replicates on the master branch too.

On your branch, there are >2000 lines of external source code added besides package own source code:

# find lib/ -name '*.js' | xargs wc -l
6185 total
 
# cat dist/markdown-it.js | wc -l
8356
 
# cat dist/esm/markdown-it.mjs | wc -l
8675

These may be duplicated in a Web App that makes use of the same dependencies like this package because the app bundler has no idea that they were already included in this package bundle.

[paulirwin]

@adrian-branescu Thanks for the info, I could see where the addition of this could exacerbate that problem. If there's consensus from others that the external bit should be included here, I can do that.

[marijnh]

@puzrin Are you okay with releasing this? Or giving some feedback on what would be needed before it gets released?

[puzrin]

We closed most of pending issues/prs, and I'm rewriting to es6 to solve the rest.

Need advice, what is preferable:

• use .mjs extensions
• use .js with type: module

Could you help?

[marijnh]

It doesn't matter much, but if you want to keep supporting the old behavior of requiring dist/markdown-it.min.js, you'll need to either use .mjs for your ES module files, or explicitly list all allowed paths in your package.json exports field, because with type: module a .min.js file would be interpreted as a module.

[puzrin]

@marijnh thank you very much for info! I tend use .msj, if that will be ok to all next years.

[puzrin]

See es6 branch https://github.com/markdown-it/markdown-it/tree/es6

• all rewritten all to es6 (dependencies still not, but will be done)
• dist/ now has umd build with all deps bundled.

Since change is huge, and I have no solid experience with esm development, I'm open to any suggestions, what should be improved. Hope to spread the same changes to all other maintained repos.

[SReject]

@puzrin

See es6 branch https://github.com/markdown-it/markdown-it/tree/es6

* all rewritten all to es6 (dependencies still not, but will be done)

* `dist/` now has umd build with all deps bundled.

Since change is huge, and I have no solid experience with esm development, I'm open to any suggestions, what should be improved. Hope to spread the same changes to all other maintained repos.

Use Class syntax

MDN Docs

So, using /lib/index.js as an example:

// function MarkdownIt
class MarkdownIt {

  /*
  function MarkdownIt(presetName, options) {
  // ...
  }
  */
  constructor(presetName, options) {
    // ...
  }
  
  /*
  MarkdownIt.prototype.set = function (options) {
    // ...
  }
  */
  set(options) {
    // ...
  }

  /*
  MarkdownIt.prototype.configure = function (presets) {
    // ...
  }
  */
  configure(presets) {
    // ...
  }
}

Use let/const

MDN Docs

Instead of declaring variables with var, use let if the variable will change, or const if it does not

[puzrin]

• Updated a lot of code to be more close to standard lint rules (var => let/const and so on).
• Added babel to es5 build, for sure.

@SReject The problem with classes is, that existing implementation uses hack for the call without new. This is not possible with class. I could export factory function as default and class as MarkdownIt. But not sure how good is that for users. Any suggestions welcome.

Also not sure, if should I use rollup+babel for the es6 modules build, or leave that for upper-level use.

[SReject]

• Updated a lot of code to be more close to standard lint rules (var => let/const and so on).

  • Added babel to es5 build, for sure.

@SReject The problem with classes is, that existing implementation uses hack for the call without new. This is not possible with class. I could export factory function as default and class as MarkdownIt. But not sure how good is that for users. Any suggestions welcome.

Its generally considered an anti-pattern now-a-days to use class-factory functions. For those that wish it, its best to export the class definition as default, then the factory function as a seperate export:

class MarkdownIt {
  // ...
}
const factory = (...args) => new MarkdownIt(...args)

export {
  MarkdownIt as default
  factory as MarkdownIt
};

// import the class, use it w/ 'new'
import MarkdownIt from 'markdown-it';
const thing = new MarkdownIt();

// or

// Import the factory function, use it without 'new'
import { MarkdownIt } from 'markdown-it';
const thing = MarkdownIt();

// or

// Import both
import MarkdownItClass, { MarkdownIt as MarkdownItFactory } from 'markdown-it';
const thing1 = new MarkdownItClass();
const thing2 = MarkdownItFactory();

[rlidwka]

Its generally considered an anti-pattern now-a-days to use class-factory functions.

What is the reason to force every user to type new?

Right now in markdown-it, invocations with and without new are equivalent. So I argue that this is purely about code style users want to have in their project. Similar to semicolons, indentations, newlines after if, etc.

---

Aside:

Callable constructors can be done with ES6 classes. And I've did that for argparse:
https://github.com/nodeca/argparse/blob/a645a9a9d3d0a347f383d0b795859e67dfae6ad8/argparse.js#L200-L212

But honestly, I think it's better to keep it as function+prototype for now, until ecmascript spec guys finally manage to come up with callable constructors out of the box without complexities like Reflect.

[SReject]

Just for clarity, when I say 'class-factory functions' I mean functions that serve a duel purpose of being both a constructor and a function that returns an instance. Now working backwards from your post...

until ecmascript spec guys finally manage to come up with callable constructors out of the box without complexities like Reflect.

Looking at proposals, this is sadly not going to happen any time soon. The one proposal to rectify such has been withdrawn in favor of decorators. The original reasoning for not having callable class constructors is the committee's views on separation of intents: (paraphrased) "A function should be callable, a class should be an instantable".

So I argue that this is purely about code style users want to have in their project. Similar to semicolons, indentations, newlines after if, etc.

This is a true and not true. While I disagree with the commitee's opinion in regards to callable class constructors, the current state of ES should inform modern coding practices; that being there should be a distinction of intents.

What is the reason to force every user to type new?

There is none nor am I suggesting such. In the example I provided, I showed how to have both.

As an aside, I definitely like your _callable() decorator as it provides the best of all worlds. ES6 class syntax with a callable constructor.

[rlidwka]

While I disagree with the commitee's opinion in regards to callable class constructors, the current state of ES should inform modern coding practices; that being there should be a distinction of intents.

This distinction of intents is not relevant for this library. In fact, the only case I've seen so far where the behavior is different is Date(), and even that case is arguably a bug that can't be fixed because of backward compatibility - nobody uses Date() as a function.

Thus, I consider it a bug in ES6 spec. And until it gets fixed, we cannot use ES6 classes.

[puzrin]

I'm closing this, since master rewritten to ESM, and have fallback to CJS (generated on publish, or via npm run build).

It will take some time (up to 1-2 weeks) to check other issues and transform plugins. But you can write any thoughts if something else should be fixed.

[puzrin]

Published v14 with native ESM support.