chore(test): refactor tests to use ESM syntax

[aduh95]

Changes

Checklist

• Added markup tests, or they don't apply here because...
• Updated the changelog at CHANGES.md

[aduh95]

@joshgoebel note that I haven't updated test/api/index.js, and the test suite is completing with code 0 instead of returning an error, we probably have to change mocha configuration for this kind of behaviour is reported as an error.

[joshgoebel]

No idea, is it an async issue?

[aduh95]

Hum apparently that was because I was using await import() instead of static import in some files. I think this is ready now.

[joshgoebel]

Now highlight.js and highlight.cjs both seem to run?

[joshgoebel]

Oh wait because you included both... is that intentional? I thought we only needed the CJS one for the one thing?

[aduh95]

It was intentional, I think we want to make sure both ESM and CJS users are able to run the library. It's more an integration test than a unit test spirit.

[joshgoebel]

Was this (and the other => => function changes) necessary or is this just a style change?

[aduh95]

Yes, because this is undefined in an ES module, so the test throws when arriving at this.blocks =. Another way of fixing it would be to declare a variable blocks in the function scope and do a s/this.blocks/blocks/ replacement.

[joshgoebel]

But when I merge our work (and rename the files back to .js) I get errors:

https://github.com/joshgoebel/highlight.js/tree/esm-tests-more-work

I find the error entirely opaque:

Writing highlight.js

> highlight.js@10.6.0 test
> mocha test


SyntaxError[ @/Users/jgoebel/work/highlight.js/test/index.js ]: Unexpected identifier
    at Loader.moduleStrategy (node:internal/modules/esm/translators:147:18)

[aduh95]

I agree the error message is unclear. From what I understand, this is due to syntax errors in the build/lib/index.js file:

import hljs from './core.js';

hljs.registerLanguage('1c', import L_1c from './languages/1c.js';);
hljs.registerLanguage('abnf', import L_abnf from './languages/abnf.js';);

It seems the require calls are being replaced by import in a "dumb" way.

[aduh95]

You need to replace

highlight.js/tools/build_node.js

Line 21 in 22ec6ff

return `hljs.registerLanguage('${lang.name}', ${require});`;

With

    return `${require};hljs.registerLanguage('${lang.name}', ${importName});`;

But then we run into the issue that the module doesn't have default export…

[aduh95]

highlight.js/tools/build_node.js

Line 15 in 22ec6ff

const importName = "L_" + lang.name.replace("-","_")

This is not very robust, it replaces only the first dash. It would be worth doing instead:

    const importName = "L_" + lang.name.replace(/\W/g,"_");

[joshgoebel]

If I import first then use the identifier as you suggest then I just hit:

Error [ERR_REQUIRE_ESM]: Must use import to load ES Module: /Users/jgoebel/work/highlight.js/build/lib/index.js
require() of ES modules is not supported.
require() of /Users/jgoebel/work/highlight.js/build/lib/index.js from /Users/jgoebel/work/highlight.js/test/api/highlight.cjs

[aduh95]

Yes, highlight.cjs test needs to be updated to use import('#hljs') instead of require('#hljs'). Do you want me to update #2 with this change?

[joshgoebel]

I thought the only reason cjs existed was to test requiring the library? But that's not possible? I'm confused, sorry.

Changing it takes you right back to

(node:86047) Warning: To load an ES module, set "type": "module" in the package.json or use the .mjs extension.
(Use `node --trace-warnings ...` to show where the warning was created)

/Users/jgoebel/work/highlight.js/test/api/highlight.cjs:4
import hljs from '#hljs';
^^^^^^

SyntaxError: Cannot use import statement outside a module

[joshgoebel]

require() of ES modules is not supported.

I thought you said this "just worked"... how can we publish an ES6 library if it can't be required by people? I feel like I'm missing something. I also thought I tried it before and it worked... so ugh.

[aduh95]

CJS users would need to import() it rather than require() it. Take a look at 2d8c9b7 to see what I'm talking about.

[joshgoebel]

Hmmm... If I remove the async/await I'm stuck with:

   1) .highlight()
       should works without continuation:
     TypeError: Cannot read property 'highlight' of undefined
      at Context.<anonymous> (test/api/highlight.cjs:7:25)
      at processImmediate (node:internal/timers:464:21)

CJS users would need to import() it rather than require()

So does this mean users must also be forced to make sure their call site is async?

[aduh95]

They can either use a bundler, switch to ESM, or make their API async.

[joshgoebel]

They can either use a bundler, switch to ESM, or make their API async.

Ugh. I believe somewhere along the line I got confused (and maybe I didn't do enough testing) that since ES6 modules were "fully supported" now this would be more of a drop-in change... that sounds potentially like a LOT of work for a lot of people. It seems like a lot less effort for the ES6 people to deal with a library being CJS than for CJS people to deal with a library switching to ES6.

I'm not sure I see the advantage of making this change for Node users on the server-side (is there any??) - and seems to force a lot of work on them. ES6 modules in the browser was never the goal here, that was only a possible side-benefit that came for free if the ecosystem was truly ready for a "painless" upgrade from CJS to module, but it's starting to feel like it isn't.

Is there something here I'm missing?

[aduh95]

Sorry I failed to set the correct expectations for the move to ESM. It is indeed a limitation with require, because it is sync it cannot load ESM modules reliably. There is an open PR on the Node.js repo to try to fix this (nodejs/node#30891), however I don't think it's gonna happen any time soon.

It seems like a lot less effort for the ES6 people to deal with a library being CJS than for CJS people to deal with a library switching to ES6.

Let me disagree, working with CJS in the browser is a pain in my experience, you have to load a bunch of code to make it work somehow, or to use a tool that do that for you, or use the UMD bundle – either way you're pushing more JS to your users and have a (subjective) inferior developer experience.
Is it really a lot of work? It's not unheard-of that a library switches to a Promise-based API on a semver-major bump, and we are not even doing that here: for folks who already use a bundler, it's going to be completely transparent.
For folks who are using CJS and can't/don't want to switch to an async model, I guess they need to keep using v10 for the time being (same as people that need/want to keep supporting Node.js v10 after it's EOL).

I'm not sure I see the advantage of making this change for Node users on the server-side (is there any??)

The two systems are very different, I think it's very subjective which are advantages and which are limitations. The key feature for me is the ability for me to use the same syntax/module loading mechanisms for both browser and server development without setting up any tool.

if the ecosystem was truly ready for a "painless" upgrade from CJS to module, but it's starting to feel like it isn't.

I tried to find surveys that would help us finding out how spread out ESM is in the ecosystem, but couldn't find anything recent enough. If I was following my guts, I'd say we should do it, CJS is clearly a legacy system that is probably going to fade out.

---

Anyway, if we are not happy with the import() experience with CJS, here's a couple of thing we can do to help smooth the transition if we stick with distributing ESM:

• Document how to transpile the library to CJS and let users deal with it. (I thought that was the way we were going for).
• Provide a CJS bundle in the npm module (E.G.: highlight.js/compat). That's something I've done with one of my package and I haven't received complains.
• Publish as a dual-CJS+ESM package. As we discussed previously, this is not ideal.

I feel quite strongly that ESM is the correct way forward, however I realize I'm the only one who actually requested it, so I would understand if highlight.js as a project chose a different path. However I'll still try to use my humble influence of mere contributor of the project to guide it towards the light path of ESM 😉

[joshgoebel]

Provide a CJS bundle in the npm module (E.G.: highlight.js/compat). That's something I've done with one of my package and I haven't received complains.

Link please, I would take a look at what that looks like.

[aduh95]

https://github.com/aduh95/viz.js#synchronous-api

[aduh95]

Note that it's not exactly the same thing, as the main API is promise-based, and I'm providing a different synchronous API, transpiled to CJS.

[joshgoebel]

Note that it's not exactly the same thing

Yeah...

Provide a CJS bundle in the npm module (E.G.: highlight.js/compat)

I was looking for an example of a library that's taken this approach that I could have a look at. Do you know of any?

[aduh95]

I was looking for an example of a library that's taken this approach that I could have a look at. Do you know of any?

Rollup does something like that, they use package.json "exports" field to hide it from end users – that's a path we could explore if that help smooth out the transition.

I've created #3 to demonstrate what it would look like for this library.