highlight.js icon indicating copy to clipboard operation
highlight.js copied to clipboard

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

Open joshgoebel opened this issue 2 years ago • 17 comments

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"?

  • Keep using v11 (for 6 months) and then "tough luck" is one potential story...
  • It would be fairly easy to add a web build that adds global.hljs = hljs // etc to the end of the build, allowing v12 users to (as a hack) use the v12 web/CDN build and modules if they must stay on CJS.

Opening up for discussion.. CC @highlightjs/core


Proposed changes if we go ESM only:

For the node build:

  • We no longer build the lib CJS folder
  • We rename the es folder to lib
  • the -ne (don't build ESM) build flag goes away
  • simplify package.json to remove all ESM + CJS stuff
  • the es files become the actual built source, rather than merely stubs as they are now

joshgoebel avatar Apr 25 '22 15:04 joshgoebel

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

joshgoebel avatar Apr 25 '22 15:04 joshgoebel

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

Trinovantes avatar Apr 29 '22 00:04 Trinovantes

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.

joshgoebel avatar Apr 29 '22 01:04 joshgoebel

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.

Trinovantes avatar Apr 29 '22 02:04 Trinovantes

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.

joshgoebel avatar Apr 29 '22 03:04 joshgoebel

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:

  • for any layout starting in the root, matching classic Node import resolution
  • for a single type definition specified via the types key, with no additional lookup from there

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!)

chriskrycho avatar Jun 02 '22 13:06 chriskrycho

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.

oldmansutton avatar Jul 20 '22 12:07 oldmansutton

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/[email protected]/es/core.js')
Screen Shot

This file:

https://www.unpkg.com/[email protected]/es/core.js

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

https://www.unpkg.com/[email protected]/lib/core.js

fregante avatar Jul 26 '22 09:07 fregante

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.

joshgoebel avatar Jul 27 '22 01:07 joshgoebel

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.

chriskrycho avatar Jul 27 '22 02:07 chriskrycho

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

  • it requires a CJS bundler (Rollup doesn’t support this out of the box, for example)
  • any bundler with import support has always been able to import CJS files, so adding this intermediate export default file wasn’t and still isn’t useful.

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 avatar Jul 27 '22 04:07 fregante

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

fregante avatar Jul 27 '22 04:07 fregante

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

  • bundlers, which you're describing
  • direct browser access, for which there is already a build
  • Node itself, which is what the ESM shim is actually targeting, and where it does provide considerable value

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. 😩

chriskrycho avatar Jul 27 '22 12:07 chriskrycho

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

fregante avatar Jul 27 '22 13:07 fregante

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!

chriskrycho avatar Jul 27 '22 13:07 chriskrycho

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

  • offering such dual entry point isn't useful to any tool, so it's just extra complexity (as seen in #3007 and the successive issues that it caused)

Node is perfectly capable of import-ing CJS files

fregante avatar Jul 27 '22 13:07 fregante

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

joshgoebel avatar Jul 28 '22 12:07 joshgoebel