highlightjs / highlight.js

JavaScript syntax highlighter with language auto-detection and zero dependencies.
https://highlightjs.org/
BSD 3-Clause "New" or "Revised" License
23.52k stars 3.57k forks source link

Discuss: Time to go ESM only? (for NPM `highlight.js` package) #3527

Closed joshgoebel closed 6 months ago

joshgoebel commented 2 years ago

This is on the v12 possibilities list: #3219. Some were pushing us to do that last year but it felt very premature at the time. Come this May it will be one year later... and I'm thinking of perhaps a summer release for v12.

Related: #2600

To be clear: this is only discussing highlight.js NPM package, not the highlightjs/cdn-release which will continue (for now) to include our default web build (CJS monolith, CJS modules, ES modules).

If we decided to do this I wonder what the "so you're stuck on CJS" story would look like other than "tough luck"?

Opening up for discussion.. CC @highlightjs/core


Proposed changes if we go ESM only:

For the node build:

joshgoebel commented 2 years ago

Actually the CDN build has long "just worked" when required on Node via the tiny hook at the bottom:

if (typeof exports === 'object' && typeof module !== 'undefined') { module.exports = hljs; }

So maybe no other changes are required? The hack for CJS support via CDN modules in Node would look something like:

const hljs = require("cdn-release/highlight.js")
global.hljs = hljs // required for CDN language modules
require("cdn-release/languages/matlab.js") // side effects via `global.hljs`

I don't think is so terrible as a "last resort" option for CJS people...

Trinovantes commented 2 years ago

I'm very interested in an ESM release so I can enable tree shaking. Even though I use maybe 3 languages, I have to bundle the entire package and it's now ~80% of my initial bundle size.

Also, you can publish both ESM/CJS packages and let node resolve the file automatically

  exports": {
    ".": {
      "require": "./index.cjs", // CJS
      "import": "./index.mjs"   // ESM
    }

A more complex example: https://github.com/vuejs/core/blob/main/packages/vue/package.json#L5-L42

joshgoebel commented 2 years ago

Not sure what your bundler is doing. You certainly don't need ESM to only bundle the grammars you use. Rollup does this just fine, just to name one.

We already publish a CJS/ESM dual package currently. This issue is regarding whether or not we drop the CJS support entirely with v12 or not.

Trinovantes commented 2 years ago

I was not aware there's already an ESM export. I just assumed it was CJS because in order for webpack to tree shake the unused files, every file needs to be an ESM import/export rather than just having an ESM wrapper around CJS modules. If the plan is to use ESM imports everywhere rather than just the index file, it would probably solve my issue.

joshgoebel commented 2 years ago

You should probably be importing core, not index. I don't think full ESM is magically going to fix your issue.

It should be easy to bundle just what you need today. ESM or CJS.

chriskrycho commented 2 years ago

Note that you cannot successfully use the core import today with TypeScript because it's published in a location that TS cannot by default resolve. Until TS 4.7 introduced support for Node's package export maps (exports in package.json), nested layouts like the one highlight.js currently uses will only resolve types in one of two ways:

What that means in practice is that a dist directory with types in it is roughly useless to end users (without a compilerOptions.paths configuration, at least) except insofar as those types are re-exported from whatever definition file is specified by the types key in package.json. As it stands, TS users who want to import from highlight.js/core must set up their tsconfig.json to resolve it manually:

{
  // ...
  "compilerOptions": {
    // ...
    "paths": {
      "highlight.js/*": ["node_modules/highlight.js/lib/*"]
    }
  }

(Leaving this here mostly for reference for other folks who end up stumbling across this!)

oldmansutton commented 2 years ago

As somebody who refuses to use bundlers and stay as pure JS as possible, actual pure ESM would be preferred. The only reason I even have node installed is for the convenience of using npm to easily find and download packages. An ESM option so that everything works in browser without bundler/node shennanigans? Yes, please.

fregante commented 2 years ago

We already publish a CJS/ESM dual package currently

The ESM file imports a CJS file that references module.exports so effectively it's just a facade:

await import('https://www.unpkg.com/highlight.js@11.6.0/es/core.js')
Screen Shot

This file:

https://www.unpkg.com/highlight.js@11.6.0/es/core.js

Imports this file (scroll to the bottom to see module.exports)

https://www.unpkg.com/highlight.js@11.6.0/lib/core.js
joshgoebel commented 2 years ago

It's a "façade" on purpose. I did quite a bit of reading at the time and from what I could determine this is/was the recommended way to publish a dual NPM package for Node.JS. If you make the ESM module truly stand alone then it becomes possible for someone to accidentally import the CJS library AND the ESM library (via different peer-dependencies). Then you have two different instances of the library when you only expect one.

The "facade" solves that problem. When CJS support is dropped the facade goes with it.

If I recall correctly our web ESM build is fully standalone as it does not have to deal with this Node.js ecosystem drama.

chriskrycho commented 2 years ago

That remains the best path today. For TS consumers, you might use the typesVersions hack to make the rest of it resolveable, but the basic interop choice is sound and much appreciated.

fregante commented 2 years ago

I get it, but it’s still pointless because:

I mean, just look at it, the ESM file imports a CJS file; any external tool that supports that can also support import 'highlight/file.cjs directly.

What I’m saying is that highlight is not a dual CJS/ESM package as you stated.

All of these reasons and the ones you stated are why I avoided and still avoid publishing mixed packages, it was an unwinnable game.

fregante commented 2 years ago

Having said that, I think publishing a “true ESM” package should be fine since all bundlers support it and highlight is mainly a browser package, from what I understand. I say this because if you publish a "type: module" package meant to be used in Node you will get a bunch of requests to revert it, see the billion comments on https://web.archive.org/web/20220218023916/https://gist.github.com/sindresorhus/a39789f98801d908bbc7ff3ecc99d99c

chriskrycho commented 2 years ago

The challenge is that there are multiple use cases for published ESM:

Also, the terminology @joshgoebel is using, describing this as a dual-ESM/CJS package, comes literally straight out of the Node docs.

None of that changes the annoyance of the interop, but that’s primarily on Node (which is where I use it, as part of my website build process, not what I bundle and ship to browsers), rather than on maintainers trying to do the least-worst thing. 😩

fregante commented 2 years ago

There seems to be a lot of confusion around this, and it's understandable given the long history of ESM, but it seems that none of you actually verified that it does provide "considerable value".

I challenge you to delete the "module" field from the package.json and tell me what exactly stops working, because I'm saying it does absolutely nothing. The CJS lib/index.js file even has a exports.default property so most versions of bundlers/babel are able to correctly pick it up.

dual-ESM/CJS package, comes literally straight out of the Node docs.

The docs purely mention setting a module field but don't specify nor care about what's inside the file, because it's completely out of their scope. Using that as "an official source" is therefore meaningless.

Once again, offering an intermediate ESM file does not equate to offering a real ESM package. You may say it's ESM, but browsers can't load it[^1], Node can't load it, Rollup can't load it… other bundlers can, and they can just as well bundle without it.

[^1]: I'm aware of the real-ESM version on CDNs, unrelated to the npm package of this discussion

chriskrycho commented 2 years ago

Node does load it. 😄 Not sure what you’re experiencing, but I have used this exact pattern in several other (internal) libraries at work successfully and have used it as a consumer of this library as well.

Also, I linked directly to the section of Node’s docs using exactly this verbiage and describing exactly this pattern for one solution to avoiding problems when publishing both versions of a package in parallel; they say considerably more than you suggest!

fregante commented 2 years ago

Darn, I had totally missed the exports in the package.json, that changes a couple of things I said. 😅 This still stands:

Node is perfectly capable of import-ing CJS files

joshgoebel commented 2 years ago

I seem to hear absolutely zero people saying "we still really need CJS"... 💯