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: Should Highlight.js version 11 npm release be ESM only? #2600

Closed
aduh95 opened this issue Jun 10, 2020 · 56 comments · Fixed by #3007
Closed

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

aduh95 opened this issue Jun 10, 2020 · 56 comments · Fixed by #3007
Labels
autoclose Flag things to future autoclose. enhancement An enhancement or new feature package/build Issues relating to npm or packaging

Comments

@aduh95
Copy link
Contributor

aduh95 commented Jun 10, 2020

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 aduh95 added the enhancement An enhancement or new feature label Jun 10, 2020
@joshgoebel
Copy link
Member

joshgoebel commented Jun 10, 2020

Support for ESM has landed in Node.js current and LTS lines recently

Landed landed vs still experimental? Do you have a link/announcement?

Is there a graph or stats anywhere of who is still using Node v10 (it's popularity still)? Is it considered "very old"? We have very limited time - I'd really rather not maintain both CJS and ESM if we can avoid it. I'd much rather say drop CJS with the next major release and switch completely to ESM if it works for the majority, etc.

@egor-rogov @allejo Thoughts?


Also worth noting the internal libs are already ESM, but our npm packages (built for Node primarily) are packaged as CJS. If you started from just the GitHub repo source you'd already have ESM all the way down.

@joshgoebel
Copy link
Member

@aduh95 Do you know if the built-in support can import from JSON files? We currently do this (via rollup) to pull in the version from the package.json file, though I suppose we could change that if we had to.

@aduh95
Copy link
Contributor Author

aduh95 commented Jun 10, 2020

Do you have a link/announcement?

https://nodejs.org/en/blog/release/v12.17.0/

It used to be behind a flag and a warning, it's still officially experimental though. If you want to ditch CJS, it might be wiser to wait for the feature to be officially stable (the goal of Node.js is to have it stable by the time v14 reaches LTS in October).

Node.js v10 is in LTS maintenance mode until April 2021, so we can expect people still using it until its end of life. I don't think there are any official download stats for Node.js versions 😕

What is the timeframe for Highlight.js 11.x?

If you started from just the GitHub repo source you'd already have ESM all the way down.

That's a very good point, although it's worth noting that the source doesn't specify file extension in the imports (E.G.: import './file' instead of import './file.js'), which makes it not suitable for Node.js, Deno, or browser consumption without transformation. Maybe that something we can discuss refactoring?

Do you know if the built-in support can import from JSON files?

Importing JSON using ESM syntax is still experimental on current Node.js version. But there are workaround for this (example).

@joshgoebel
Copy link
Member

What is the timeframe for Highlight.js 11.x?

Early 2021 prolly. I don't like to push breaking releases too often... so my idea is a roughly yearly cycle... but I'm also not in a rush if we don't NEED to break anything - 10 could live on longer.

it's still officially experimental though

Yeah, I'd say it's not a super high priority for us while it's still experimental.

it's worth noting that the source doesn't specify file extension in the imports (E.G.: import './file' instead of import './file.js')

If Rollup doesn't mind I'd have no issue with adding .js everywhere.

@joshgoebel joshgoebel added the package/build Issues relating to npm or packaging label Jun 12, 2020
@joshgoebel joshgoebel added this to the 11.0 milestone Jun 12, 2020
@joshgoebel joshgoebel mentioned this issue Jun 12, 2020
25 tasks
@joshgoebel
Copy link
Member

joshgoebel commented Jun 12, 2020

Is anyone using .jsm? Or a special extension for modules vs normal CJS?

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

Perhaps I should have looked at this a bit more carefully. Is this really ALL you were looking for here? If we shipped those 2-3 build assets (or variants of) with ONLY this change... would that resolve this? I was imagining publishing all the "raw" source files as their own or alternative NPM package... which is a whole other level of complexity.

Just shipping ESM "entry points" for core and index sounds perhaps a lot more doable.

@aduh95
Copy link
Contributor Author

aduh95 commented Jun 12, 2020

Is anyone using .jsm? Or a special extension for modules vs normal CJS?

.mjs is the one recognized by Node.js.

would that resolve this?

Yes, that would be totally fine! I think Rollup can handle that, right?

output: { format: "cjs", strict: false },

Haven't tried it, but I think you can replace it with:

output: [
  { format: "cjs", strict: false },
  { format: "esm" },
]

Would you interested in a PR?

@joshgoebel
Copy link
Member

Actually that will mess with all the individual languages we require also, no? Those would then also need to also be modules, yes? That's what I was fearful of... I'm not sure we want to ship +180 additional/duplicate JS files just because of ESM... is that the only way to do this - or am I thinking about it wrong? I wonder if there are any other large modular libraries and how they are dealing with this problem?

Building a "monolith" (like we do for the browser) would solve some of the issues [for some people] but others really prefer to load just core and the languages they need one at a time, and it seems to allow that we'd still need 100s of separate modules.

@joshgoebel
Copy link
Member

but I think you can replace it with:

I doubt it. We have a pretty custom build process. It'd probably be easier to build a list of the individual CJS files and then feed them thru a simple text transform... then spit out new files beside them.

But we'd also preferably want our CI/test suite to test these new "build products" and I'm not sure that's even even possible with Mocha. Do you know? Last time I looked at using modules with it things seemed pretty rocky... but perhaps that has improved?

@joshgoebel
Copy link
Member

joshgoebel commented Jun 12, 2020

@aduh95 If your actual problem is truly just "get started fast" why not just use the browser build and change it's single CJS export to an ESM export instead?

@aduh95
Copy link
Contributor Author

aduh95 commented Jun 12, 2020

Thanks for the detailed explanation, I think I got your point. This discussion would be more productive in a few months when Node.js ESM support has stabilised.

Maybe we can wait and see if the situation evolves on Node.js side, and come back to the "drop CJS with the next major release and switch completely to ESM" mindset when it makes sense?

But we'd also preferably want our CI/test suite to test these new "build products" and I'm not sure that's even even possible with Mocha. Do you know?

Yes Mocha is able to interact with ES modules, you just have import() them instead on require() them.

If your actual problem is truly just "get started fast" why not just use the browser build and change it's single CJS export to an ESM export instead?

I want the best of both world: get started fast and cherry-pick the languages I'm actually highlighting. I know I'm bad ^^

@joshgoebel
Copy link
Member

This discussion would be more productive in a few months when Node.js ESM support has stabilised.

So be it.

@joshgoebel
Copy link
Member

This discussion would be more productive in a few months when Node.js ESM support has stabilised.

How are things looking now? I'm curious if other packages (with lots of individual files) are shipping BOTH ESM and CJS versions via npm and if so how they are doing it...

I'm thinking of rolling v11 perhaps in April/May and seems a good time to revisit this...

@aduh95
Copy link
Contributor Author

aduh95 commented Feb 14, 2021

Hey! Yes, there are a few packages that have started doing that, on the top of my head I can think of Rollup and Preact, I know there are more. I'm not sure they qualify for the lot of individual files part though.

On Node.js side, ES modules support is no longer experimental (nodejs/node#35781) and is considered stable. Node.js v10.x is planned to go end-of-life at the end of April 2021, which would mean all maintained Node.js release lines (v12.x, v14.x, v16.x) would have support for ES modules.

IMHO, considering the time frame, I think we should go all-in with ESM and ditch CJS in the npm package:

  • A user working with CJS modules can still use highlight.js using import('highligh.js').then().
  • Users working on web apps could start using highlight.js without a bundler/transpiler.
  • That wouldn't make the npm package twice as heavy (which would inevitably happen if we were to ship both CJS & ESM).

I can work on a PR to start experimenting with that if that sounds like a plan to you.

@joshgoebel
Copy link
Member

A user working with CJS modules can still use highlight.js using import('highligh.js').then().

Does this also then work perfectly in v12? Did the ESM support become non-experimental in the latest releases of v12 or do you still need to trigger a special runtime flag?

Users working on web apps could start using highlight.js without a bundler/transpiler.

You mean vs using our pre-built and minified distributable assets? (which we'll still create for users who need/prefer them) I'm all for giving people choices, but are there big advantages using ESM if the library isn't built with that in mind? Someone crazy enough to import everything would be loading/fetching over 180 distinct JS files. It's possible I don't understand the full ESM in-browser story yet since I've always bundled on every web app I've worked on.

That wouldn't make the npm package twice as heavy (which would inevitably happen if we were to ship both CJS & ESM).

I'm not super worried about the size, thought it is a consideration. More worried about maintaining dual build, and CI pipelines... but the end result is the same: A single final product be nicer than two. :)

I can work on a PR to start experimenting with that if that sounds like a plan to you.

Is there a reason the npm build process couldn't mostly do a cp **/*.js -> into proper places since we're already using ESM modules for our raw source? I also need to take another look at the mocha story for ESM now...

@aduh95
Copy link
Contributor Author

aduh95 commented Feb 15, 2021

Does this also then work perfectly in v12? Did the ESM support become non-experimental in the latest releases of v12 or do you still need to trigger a special runtime flag?

Yes, v12.20.0 has the same implementation as current v15.x (with the exception of top-level await, but that's because it's using an older version of V8), no flag is needed, and the experimental warning has been removed.

Someone crazy enough to import everything would be loading/fetching over 180 distinct JS files.

In some projects of mine I was using highlight.js to highlight one specific language, so that would be only two files to fetch, correct? Otherwise, I agree you should use a bundler or the pre-built file if you want to include everything.

Is there a reason the npm build process couldn't mostly do a cp **/*.js -> into proper places since we're already using ESM modules for our raw source?

Yes probably, since a010c15 that should be possible. There some documentation and update to make to package.json (we need to add "type": "module" so Node.js and bundler know we're shipping ESM), but yeah, hopefully it should be pretty straightforward :)

@joshgoebel
Copy link
Member

joshgoebel commented Feb 15, 2021

Looks like the modules themselves almost work as-is (except for our importing of package.json) with a small test utility I use a lot... Too bad this seems to require --experimental-json-modules https://stackoverflow.com/questions/59738999/does-the-es-modules-implementation-support-importing-json-files Is there some nice solution that I'm missing of if I want to magically import the version number from package.json into the library?

In some projects of mine I was using highlight.js to highlight one specific language, so that would be only two files to fetch, correct?

Well our raw source tree is broken up into separate files for organization. Just the library + one languages is maybe 7-8 different small files. Perhaps (for API privacy concerns as well) we still need to build cooked ESM assets for:

  • core (exports just the lib)
  • common (imports core + registers common grammars, exports core)
  • index/all (imports core + registers all grammars, exports core)
  • languages/* (exports individual grammars)

Thoughts?

@aduh95
Copy link
Contributor Author

aduh95 commented Feb 15, 2021

Is there some nice solution that I'm missing of if I want to magically import the version number from package.json into the library?

On Node.js, you can do that:

import { readFile } from 'fs/promises';

const { version } = await readFile(new URL('../package.json', import.meta.url)).then(JSON.parse);
console.log(version); // Outputs '10.6.0'.

Doing so would not work on the browser, so that's probably not the way to go for the distributed package…

Given that plus the fact that the source files are broken up into several files, it seems the way forward would be to update the build_node script to output ESM, and keeping the current distributed file organization (except it would contain ESM instead of CJS).

@aduh95
Copy link
Contributor Author

aduh95 commented Feb 15, 2021

Too bad this seems to require --experimental-json-modules

FYI it's expected to stop being necessary soon: nodejs/node#37141.

@joshgoebel
Copy link
Member

joshgoebel commented Feb 15, 2021

@aduh95 If you wanted to help further you could take a look at that PR and provide any thoughts. Also if you wanted to muck around with getting the test suite fully operational again that'd be awesome. Best case I'd guess it's changing a bunch of requires to imports, but I'm not really sure?

Looks pretty clean so far.

@aduh95
Copy link
Contributor Author

aduh95 commented Mar 23, 2021

I guess you're right, if we keep the same file structure for CJS module and add exports conditional exports to rewrite ESM resolution and opt-out of package encapsulation, I don't see why it would be breaking.

@joshgoebel
Copy link
Member

And what would the downsides of such an approach be?

@aduh95
Copy link
Contributor Author

aduh95 commented Mar 24, 2021

I don't think there is any downside, in the OP I was suggesting adding ESM as a non-breaking change using conditional exports so it seems we've just circled back :)

@joshgoebel joshgoebel removed the discuss/propose Proposal for a new feature/direction label Mar 24, 2021
@joshgoebel joshgoebel removed this from the 11.0 milestone Mar 24, 2021
@joshgoebel
Copy link
Member

Sometimes we take the long way around. :-) I'll take another swing at this in a bit with this goal in mind.

@joshgoebel
Copy link
Member

Can you have a look at #3007 now?

@wooorm
Copy link

wooorm commented Mar 27, 2021

@joshgoebel FYI: I’ve started changing the modules that I maintain to ESM-only. Starting from the low-level projects, and reaching lowlight somewhere next month probably. Let me know if I can advise on something

@joshgoebel
Copy link
Member

@wooorm ESM-only sounded too ambitious to me so I think we're likely to ship a dual library with conditional exports. If you know anything about that and wanted to take a glance at #3007...

@wooorm
Copy link

wooorm commented Mar 27, 2021

@joshgoebel I tried that with micromark in November and users ran into a ton of problems with tools, such as webpack 4 (still used in CRA and Next): remarkjs/react-markdown#518. I’m not optimistic that you can push dual esm/cjs in a minor release.
A way around it would be if you’d eliminate default exports. Maybe there’s other ways too

@joshgoebel
Copy link
Member

How did you try it though? I was planning to leave the core library/package.json still CJS (type: commonjs)... we're adding ESM to the es folder... so any bundler who "wasn't aware" of the "new" things would just see the CJS and work as it always had - or that's my thinking. Now if a packager is mis-handling the new standards - then I'd say that's kind of a packager issue?

Looks like you said Webpack fixed their issues in version 5?

And we're gearing up for v11, so it doesn't have to be a minor release, I just thought if we could divorce it from v11 that'd be one less thing, but if it can't be, that's fine also.

@wooorm
Copy link

wooorm commented Mar 27, 2021

micromark/micromark#36 is the main chunk of the work.

so any bundler who "wasn't aware" of the "new" things would just see the CJS and work as it always had - or that's my thinking.

I had the same idea. It turned out to be incorrect. Bundlers and other tools have been running faux-ESM for so long, that they never bothered to make them spec compliant / work.

Now if a packager is mis-handling the new standards - then I'd say that's kind of a packager issue?

Agreed.

Looks like you said Webpack fixed their issues in version 5?

Yep. But lots of users are still on broken tools.

@joshgoebel
Copy link
Member

joshgoebel commented Mar 27, 2021

Bundlers and other tools have been running faux-ESM for so long

So if someone was literally using our canonical requires:

const hljs = require('highlight.js');
// ... or ...
const hljs = require('highlight.js/lib/core'); 

Would those still break even assuming index.js and core.js had 0 actual changes? Or is the breakage for people already using faux-ESM with bundlers and switching to REAL ESM?

@wooorm
Copy link

wooorm commented Mar 27, 2021

I believe that’s indeed the issue. Webpack 4 will “magically” start using ESM in your above example, even thought it’s clearly CJS expecting CJS, and it doesn’t understand that actual ESM 🤷‍♂️ (edit: to clarify, something in between your 2 cases)
You might get around it by adding an __esModule: true and a .default: theDefaultExport field, but I’m not 100% sure off the top of my head. You should test that 😅

@joshgoebel
Copy link
Member

joshgoebel commented Apr 5, 2021

How can you still support JS requires with file extensions using conditional exports subpath patterns?

const swift = require('highlight.js/lib/languages/swift.js')
Error: Cannot find module '/Users/jgoebel/work/tmp_app/node_modules/highlight.js/lib/languages/swift.js.js'

It always wants to re-add the extension, even if it's already there. And I think using/requiring extensions is a very common pattern, there is even a linter setting to require them.

@wooorm
Copy link

wooorm commented Apr 6, 2021

That depends on the exports? I haven’t used it really, but here’s the docs: https://nodejs.org/api/packages.html#packages_subpath_exports.

@aduh95
Copy link
Contributor Author

aduh95 commented Apr 6, 2021

I think that would be a breaking change indeed. We could hack our way around it by adding a swift.js.js that contains

function emitWarning() {
  if (!emitWarning.warned) {
    emitWarning.warned = true;
    process.emitWarning(
      'Using file extension in specifier is deprecated, use "highlight.js/lib/languages/swift" instead'
    );
  }
}
emitWarning();
exports = require('./swift.js');

@joshgoebel
Copy link
Member

That depends on the exports? I haven’t used it really, but here’s the docs:

I've read them. They don't seem to address this that I see other than acknowledging the problem at one point. I was posting it here hoping someone knows the solution. Optimally there would be a way to make it "just work" extension or not - without having to have a manual list of 180 languages in exports.

@joshgoebel
Copy link
Member

We could hack our way around it by adding a swift.js.js that contains

Ha, indeed. Before we went there though I think I'd just bloat the package.json with 180 entries for every language and give up on *.

@aduh95
Copy link
Contributor Author

aduh95 commented Apr 6, 2021

I don't think that fit your use-case, but I thought I should mention it anyway: you could also change package.json to have limited support for extension paths:

  "exports": {
    "./lib/languages/*": { "require": ["./lib/languages/*.js", "./lib/languages/*"], ... }
  },

That would still fail for Node.js (see nodejs/node#37928 (comment)), but would work for some bundlers – I know that works in Webpack 5, not sure for others. Depending on how breaking you are wanting to go with this, it may be an acceptable compromise.

@joshgoebel
Copy link
Member

"./lib/languages/*": { "require": ["./lib/languages/*.js", "./lib/languages/*"], ... }

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

Yes the "exports" field was designed for package encapsulation first, for package authors who want to be picky regarding what is exposed to their users.

I wonder if we shouldn't just forget exports then - we only publish things we want to expose in any case. I just thought it was nice that it made ESM "just work" with the same paths... (loading the ESM modules vs CJS)... if we remove exports then ESM users either have to change "lib" to "es"... or they'll just be importing the existing CJS modules, which actually works just fine if the environment is Node.

@aduh95
Copy link
Contributor Author

aduh95 commented Apr 6, 2021

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

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

aduh95 commented Apr 6, 2021

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

joshgoebel commented Apr 6, 2021

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

@joshgoebel joshgoebel added the autoclose Flag things to future autoclose. label Apr 13, 2021
@joshgoebel
Copy link
Member

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autoclose Flag things to future autoclose. enhancement An enhancement or new feature package/build Issues relating to npm or packaging
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants