chore(test): refactor tests to use ESM syntax #3011

aduh95 · 2021-02-15T19:34:43Z

Changes

Refactor all tests to ESM, except one that is using CJS to ensure CJS API still works.

This is setting "type": "module" to avoid using .mjs extension, that could come in handy when doing #3007.

Josh Checklist

make sure browser build package.json isn't interfering with CDN package.json

Checklist

Added markup tests, or they don't apply here because...
Updated the changelog at CHANGES.md
Added myself to AUTHORS.txt, under Contributors

joshgoebel · 2021-02-15T20:01:40Z

This is setting "type": "module" to avoid using .mjs extension

Is this typically preferred?

joshgoebel · 2021-02-15T20:15:47Z

test/index.js

 hljs.debugMode(); // tests run in debug mode so errors are raised

 // Tests specific to the API exposed inside the hljs object.
 // Right now, that only includes tests for several common regular expressions.
-require('./api');
+import './api/index.js';


Import can't look into directories using the index trick without a package.json?

index.js lookup is deprecated in Node.js (https://nodejs.org/api/deprecations.html#DEP0151) for ESM, and not supported in the browser.

joshgoebel · 2021-02-15T20:18:01Z

Thanks so much. Will take a closer look at this later. To be clear does this break building/testing on Node 10 or is there a flag that can be used even with Node 10 to make it work after these changes?

aduh95 · 2021-02-15T21:50:49Z

Is this typically preferred?

I think it's up to your personal taste, I think both .mjs and "type": "module" are widely supported nowdays, I picked the "type": "module" approach because I thought it would be the least effort path. But I'm not so sure that was a smart call 😅:

Setting "type": "module" forced me to rename the ESLint files, and to add almost empty package.json files to folders containing CJS modules. That's not ideal.
.mjs should be a bit more efficient in Node.js as it doesn't need to lookup for the closest package.json to know it's ESM.
.mjs could even work in Node.js v10.x and v8.x with the --experimental-modules CLI flag (although I wouldn't recommand anyone using this flag, the API is very not stable on v10.x).

If you think .mjs is preferable, I can update this PR.

To be clear does this break building/testing on Node 10 or is there a flag that can be used even with Node 10 to make it work after these changes?

No, Node.js v10 wouldn't be able to resolve the #hljs import specifier (it's only supported on Node.js v12.19.0), so it wouldn't pass the test suite. Note that I used that specifier because of the discussion regarding ditching v10.x support, and I find it way cleaner to read, but there's nothing that forces us to use this feature.

joshgoebel · 2021-02-16T04:31:32Z

Note that I used that specifier because of the discussion regarding ditching v10.x support, and I find it way cleaner to read, but there's nothing that forces us to use this feature.

It's quite neat, and I had not seen it before. I was asking because for a moment I got excited and thought we could merge this part of things ASAP (as the output is still a CJS npm package)... but if we have people who are still using Node v10 as part of a build pipeline (building us from source)... then this PR would need to be held until it goes EOL...

Maybe that was always a given though since I don't think forcing people to use experimental is a good idea. Node v10 is in a weird spot for us. We don't run CI against it, but we also have fixed issues in the past... and it's also still a supported maintenance release, of course.

I wonder if we could say "You now need v12 to build us from source or run the test suite, but our npm packages still work fine with v10."... would we consider that a breaking change in a minor release? Is our build system itself include in semver guarantees, or does that really only cover our npm and browser distributables?

/cc @highlightjs/core

I picked the "type": "module" approach because I thought it would be the least effort path. But I'm not so sure that was a smart call

I don't know... may need to look around here a bit more at what other's are doing. I'm not sure efficiency (it's hard to imagine the diff is huge) or v8 and v10 support (via experimental) are big considerations - only mention v10 in my question above in regards to how long we'll need to hold this PR. Your first point about renaming lint files and more overhead with package.json seems more annoying.

I will see if I can get a feeling for which way the wind is blowing since I don't have a strong opinion on this right now.

joshgoebel · 2021-02-16T05:01:41Z

tools/build_browser.js

+  tasks.push(fs.writeFile(`${process.env.BUILD_DIR}/package.json`, JSON.stringify({
+    type:'commonjs'
+  }), { encoding: "utf8" }));


In what cases would this be used?

We already have a cdn build target that publishes a package.json... (for our cdn-assets npm package) I wonder if it's not this package.json that perhaps needs updating? Are browsers really looking for package.json or is this more informational?

Typically one builds browser only to get an individual JS file that they plan to copy elsewhere - it's not part of any "package" in any real sense - it's never published to npm, etc.

I added it because the tests with browser -n were failing (it was trying to load the UMD as ESM, which cannot work), that's another perk of having "type":"module" on the root package.json. On the other hand, it's a very small file and were don't have to include it in the distributable archive, it just needs to be there to run the test suite.

Understood. I just need to double check that this isn't interfering with the CDN build's package.json.

joshgoebel · 2021-02-18T14:21:02Z

package.json

@@ -22,10 +22,15 @@
  },
  "main": "./lib/index.js",
  "types": "./types/index.d.ts",
+  "imports": {
+    "#hljs": "./build/lib/index.js",


Does this work even without my other PR? IE, if this is still all CJS? I don't see where this is based on that, so I'm assuming so?

Yes, subpath imports are supported in Node.js v12.19.0+ and v14.6+, for both CJS and ESM.

allejo · 2021-02-22T18:00:17Z

but if we have people who are still using Node v10 as part of a build pipeline (building us from source)... then this PR would need to be held until it goes EOL...

Node 10 has an EOL of April of this year, I think it'd be safe to start dropping support for it in an official capacity.

I wonder if we could say "You now need v12 to build us from source or run the test suite, but our npm packages still work fine with v10."... would we consider that a breaking change in a minor release? Is our build system itself include in semver guarantees, or does that really only cover our npm and browser distributable

I would lean towards having our BC-guarantees only for the public API (i.e. the distributed package via npm or CDNs). As for development builds, I think at least minor bumps for new minimum versions for building packages.

joshgoebel · 2021-02-22T18:19:56Z

I think it'd be safe to start dropping support for it in an official capacity.

Yeah my current plan is to hold most of this stuff until after 10.7... we do have some good grammar work happening right now. That would make 10.7 a pretty nice spot for anyone who "stuck" on Node v10 longer than they should be. And then we open the gates with the idea that the next release would be npm ESM, require Node v12 for building, etc... i.e. a March release of 10.7 then a May release of v11.

joshgoebel · 2021-03-17T12:45:56Z

test/special/document.js

+// Setup hljs environment
+hljs.configure({ tabReplace: '    ' });
+let blocks = document.querySelectorAll('pre code');
+blocks.forEach(hljs.highlightBlock);


Suggested change

blocks.forEach(hljs.highlightBlock);

blocks.forEach(hljs.highlightElement);

joshgoebel · 2021-03-17T12:46:11Z

test/special/document.js

+hljs.configure({ useBR: true });
+
+blocks = document.querySelectorAll('.code');
+blocks.forEach(hljs.highlightBlock);


Suggested change

blocks.forEach(hljs.highlightBlock);

blocks.forEach(hljs.highlightElement);

joshgoebel · 2021-03-17T12:51:53Z

Can you please rebase this on latest master and then I'll see if I can cherry-pick onto version_11... or if you wanted to just rebase onto that yourself: https://github.com/joshgoebel/highlight.js/tree/version_11

I've removed a few files, but otherwise should be simple enough I hope.

After we release 10.7 then the version_11 branch will be up for getting merged.

joshgoebel · 2021-04-16T16:01:48Z

I'm less excited about this since the loss of namespacing for tests bugs me and there doesn't seem to be any real advantage to using modules vs not for testing. So this may just fade away (for now), but still thanks for all the help and motivation regarding ES in general.

Actually I may try to save the portion of this that applies to tools, that'd be nice I think.

Oh but this didn't touch those pieces, only testing...

aduh95 · 2021-04-19T12:47:54Z

Actually I may try to save the portion of this that applies to tools, that'd be nice I think.

@joshgoebel would you like a PR to use ESM syntax in the tools folder?

joshgoebel · 2021-04-19T12:55:24Z

No. I spent 1 minute on it and it won't work because it wants the file to be mjs or the whole package to be a module - which isn't what we're doing yet. I don't want to rename the files and we're still a CJS module (at the package.json level) so I think we just stay how it is for now?

aduh95 · 2021-04-19T12:58:02Z

Yes agreed, if there are no plans to switch to ESM only fot the time being, it makes sense to keep using CJS for now :)

joshgoebel reviewed Feb 15, 2021

View reviewed changes

joshgoebel reviewed Feb 16, 2021

View reviewed changes

joshgoebel reviewed Feb 18, 2021

View reviewed changes

joshgoebel added this to the 11.0 milestone Feb 18, 2021

joshgoebel reviewed Mar 17, 2021

View reviewed changes

chore(test): refactor tests to use ESM syntax

f186a30

aduh95 force-pushed the esm-tests branch from 0c32512 to f186a30 Compare March 17, 2021 21:44

Base automatically changed from master to main March 18, 2021 17:26

joshgoebel self-assigned this Apr 5, 2021

joshgoebel added the autoclose Flag things to future autoclose. label Apr 16, 2021

joshgoebel closed this Apr 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(test): refactor tests to use ESM syntax #3011

chore(test): refactor tests to use ESM syntax #3011

aduh95 commented Feb 15, 2021 •

edited by joshgoebel

Loading

joshgoebel commented Feb 15, 2021

joshgoebel Feb 15, 2021 •

edited

Loading

aduh95 Feb 15, 2021

joshgoebel commented Feb 15, 2021

aduh95 commented Feb 15, 2021 •

edited

Loading

joshgoebel commented Feb 16, 2021 •

edited

Loading

joshgoebel Feb 16, 2021 •

edited

Loading

aduh95 Feb 16, 2021 •

edited

Loading

joshgoebel Feb 18, 2021

joshgoebel Feb 18, 2021 •

edited

Loading

aduh95 Feb 18, 2021

allejo commented Feb 22, 2021

joshgoebel commented Feb 22, 2021

joshgoebel Mar 17, 2021

joshgoebel Mar 17, 2021

joshgoebel commented Mar 17, 2021 •

edited

Loading

joshgoebel commented Apr 16, 2021 •

edited

Loading

aduh95 commented Apr 19, 2021

joshgoebel commented Apr 19, 2021

aduh95 commented Apr 19, 2021

	blocks.forEach(hljs.highlightBlock);
	blocks.forEach(hljs.highlightElement);

chore(test): refactor tests to use ESM syntax #3011

chore(test): refactor tests to use ESM syntax #3011

Conversation

aduh95 commented Feb 15, 2021 • edited by joshgoebel Loading

Changes

Josh Checklist

Checklist

joshgoebel commented Feb 15, 2021

joshgoebel Feb 15, 2021 • edited Loading

Choose a reason for hiding this comment

aduh95 Feb 15, 2021

Choose a reason for hiding this comment

joshgoebel commented Feb 15, 2021

aduh95 commented Feb 15, 2021 • edited Loading

joshgoebel commented Feb 16, 2021 • edited Loading

joshgoebel Feb 16, 2021 • edited Loading

Choose a reason for hiding this comment

aduh95 Feb 16, 2021 • edited Loading

Choose a reason for hiding this comment

joshgoebel Feb 18, 2021

Choose a reason for hiding this comment

joshgoebel Feb 18, 2021 • edited Loading

Choose a reason for hiding this comment

aduh95 Feb 18, 2021

Choose a reason for hiding this comment

allejo commented Feb 22, 2021

joshgoebel commented Feb 22, 2021

joshgoebel Mar 17, 2021

Choose a reason for hiding this comment

joshgoebel Mar 17, 2021

Choose a reason for hiding this comment

joshgoebel commented Mar 17, 2021 • edited Loading

joshgoebel commented Apr 16, 2021 • edited Loading

aduh95 commented Apr 19, 2021

joshgoebel commented Apr 19, 2021

aduh95 commented Apr 19, 2021

aduh95 commented Feb 15, 2021 •

edited by joshgoebel

Loading

joshgoebel Feb 15, 2021 •

edited

Loading

aduh95 commented Feb 15, 2021 •

edited

Loading

joshgoebel commented Feb 16, 2021 •

edited

Loading

joshgoebel Feb 16, 2021 •

edited

Loading

aduh95 Feb 16, 2021 •

edited

Loading

joshgoebel Feb 18, 2021 •

edited

Loading

joshgoebel commented Mar 17, 2021 •

edited

Loading

joshgoebel commented Apr 16, 2021 •

edited

Loading