highlightjs / highlight.js

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

Discuss: Should Highlight.js version 11 npm release be ESM only? #2600

Closed aduh95 closed 3 years ago

aduh95 commented 4 years ago

Is your request related to a specific problem you're having?

I'm always frustrated when starting a project, I don't want to set up a whole build chain and just want to get started:

import highlight from './node_modules/highlight.js/lib/core.js'

And I get an error Error: 'default' is not exported by /node_modules/highlight.js/lib/core.js...

The solution you'd prefer / feature you'd like to see added...

I'd much rather prefer having standard ES6 modules in the npm package, and being able to use them as advertised in the README.md.

Any alternative solutions you considered...

s/module.exports =/export default/ on the node_modules/highlight.js/ files, but that's not very convenient.

Additional context...

Support for ESM has landed in Node.js current and LTS lines recently, with features such as Conditional Exports (useful in case we want to keep the CJS version around, still required for Node.js v10 support).

Having ESM files would also make the library compatible with Deno for free, which would be nice.

aduh95 commented 3 years ago

What does having require be an array do??? I haven't seen that.

See https://webpack.js.org/guides/package-exports/#alternatives:

Instead of providing a single result, the package author may provide a list of results. In such a scenario this list is tried in order and the first valid result will be used.


I wonder if we shouldn't just forget exports then

I'll take that over the current CJS-only package, but… There are breaking changes in v11 anyway, and AFAIK highlight.js has never documented using extension to load the individual language files: if that was up to me, I would go either with the array approach and document it as a breaking change, or either with adding .js.js files approach; removing the extension is doable with a simple replacement command after all, IMHO it's acceptable to put the burden on the users, but that's just me. (OK, full disclosure, I would probably go ESM-only depending how much caffeine I had on that day, but we've discussed that already :))

Again, it depends what vision you have for highlight.js v11, how much breakage you want to allow, and what status you want to give to ESM in the npm package.

joshgoebel commented 3 years ago

https://webpack.js.org/guides/package-exports/#alternatives:

Ok, but what does Node do with such constructs?

There are breaking changes in v11 anyway,

Yes, but if I got this to a "very happy with" point I was considering perhaps pushing a minor release of 10 that's dual ESM/CJS just to get a heads up on packaging issues.

has never documented using extension to load the individual language files

True, but that doesn't mean it isn't done - and I'd prefer not to break it if avoidable. Our very own eslint config requires extensions for imports. Though I'm aware this is different than requires.

IMHO it's acceptable to put the burden on the users

For v11, yes. For a slipstream v10 release, less so.

how much breakage you want to allow,

We're breaking enough other things, so it'd be nice if the module thing "just worked". :-)


What is the downside to just listing all 180 languages (with .js) explicitly in the exports? This seems to work. And we generate the exports list programmatically anyways...

aduh95 commented 3 years ago

Ok, but what does Node do with such constructs?

It would only use the first one basically, and ignore what comes after; in our case, Node.js would still throw a swift.js.js not found error

Yes, but if I got this to a "very happy with" point I was considering perhaps pushing a minor release of 10 that's dual ESM/CJS just to get a heads up on packaging issues.

Oh I see, in that case I agree, it's probably safer to avoid "exports" altogether in v10.

What is the downside to just listing all 180 languages (with .js) explicitly in the exports? This seems to work. And we generate the exports list programmatically anyways...

The upside of having .js.js files is you can add deprecation warning, and then remove the extra files in the next semver-major; but if you're happy with supporting both entry points, listing them all should work just fine.

joshgoebel commented 3 years ago

The upside of having .js.js files is you can add deprecation warning, and then remove the extra files in the next semver-major;

Ah, now that indeed make sense. So something like 1d20d88a.

joshgoebel commented 3 years ago

Tagging to autoclose. Next release will NOT be ESM only. We'll include ESM in an es folder beside lib and users will either have to reference it explicitly or we'll use conditional exports (remains to be seen which way we go).

joshgoebel commented 3 years ago

@aduh95 Merged (with conditional exports for now). Should be released when v11-alpha1 is ready.