Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
Tracked by #3219
joshgoebel opened this issue Apr 25, 2022 · 17 comments
Closed
Tracked by #3219

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

joshgoebel opened this issue Apr 25, 2022 · 17 comments
Labels
discuss/propose Proposal for a new feature/direction help welcome Could use help from community
Milestone

Comments

@joshgoebel
Copy link
Member

joshgoebel commented Apr 25, 2022

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 joshgoebel added help welcome Could use help from community discuss/propose Proposal for a new feature/direction labels Apr 25, 2022
@joshgoebel joshgoebel changed the title Discuss: Time to go ESM only? Discuss: Time to go ESM only? (for NPM highlight.js package) Apr 25, 2022
@joshgoebel
Copy link
Member Author

joshgoebel commented Apr 25, 2022

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 joshgoebel added this to the 12.0 milestone Apr 25, 2022
@Trinovantes
Copy link
Contributor

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
Copy link
Member Author

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
Copy link
Contributor

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
Copy link
Member Author

joshgoebel commented Apr 29, 2022

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
Copy link
Contributor

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

@oldmansutton
Copy link

oldmansutton commented Jul 20, 2022

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
Copy link

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

@joshgoebel
Copy link
Member Author

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
Copy link
Contributor

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
Copy link

fregante commented Jul 27, 2022

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
Copy link

fregante commented Jul 27, 2022

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
Copy link
Contributor

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

@fregante
Copy link

fregante commented Jul 27, 2022

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 it1, Node can't load it, Rollup can't load it… other bundlers can, and they can just as well bundle without it.

Footnotes

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

@chriskrycho
Copy link
Contributor

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
Copy link

fregante commented Jul 27, 2022

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

@joshgoebel
Copy link
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss/propose Proposal for a new feature/direction help welcome Could use help from community
Projects
None yet
Development

No branches or pull requests

5 participants