Support develoment environment using Node.js 19

[yan12125]

Is this a feature request or a bug?

A feature request

What is the current behavior?

I was testing if web-ext works with the latest Node.js or not, and noticed some tests failed

$ CI_SKIP_FLOWCHECK=y npm test
(...some other logs...)
  1) build
       checks locale file for malformed json:
     AssertionError: expected 'Error parsing messages.json file at /…' to match /Unexpected string in JSON at position 14/
      at file:///build/web-ext/src/web-ext-7.3.1-build/tests/unit/test-cmd/test.build.js:187:16
  2) util/manifest
       getValidatedManifest
         reports an error for invalid manifest JSON:
     AssertionError: expected 'Error parsing manifest.json file at /…' to include 'Unexpected token "," (0x2C) in JSON a…'
      at file:///build/web-ext/src/web-ext-7.3.1-build/tests/unit/test-util/test.manifest.js:36:16
      at file:///build/web-ext/src/web-ext-7.3.1-build/src/errors.js:1047:14

What is the expected or desired behavior?

web-ext works fine on Node.js 19.x and all tests pass.

Version information (for bug reports)

• Firefox version: irrelevant, installation issue
• Your OS and version: Arch Linux latest
• Paste the output of these commands:

$ node --version && npm --version && web-ext --version
v19.1.0
8.19.2
7.3.1

Logs are from web-ext 7.3.1. The issue still persists with web-ext 7.4.0.

[yan12125]

Looks like those failures are caused by changed error messages from JSON.parse(). Tests can pass with the following changes:

diff --git a/tests/unit/test-cmd/test.build.js b/tests/unit/test-cmd/test.build.js
index 9a63278..97e1295 100644
--- a/tests/unit/test-cmd/test.build.js
+++ b/tests/unit/test-cmd/test.build.js
@@ -269,7 +269,7 @@ describe('build', () => {
           assert.instanceOf(error, UsageError);
           assert.match(
             error.message,
-            /Unexpected string in JSON at position 14/
+            /Expected ':' after property name in JSON at position 14/
           );
           assert.match(error.message, /^Error parsing messages\.json/);
           assert.include(error.message, messageFileName);
diff --git a/tests/unit/test-util/test.manifest.js b/tests/unit/test-util/test.manifest.js
index 7e9e17a..5fc665e 100644
--- a/tests/unit/test-util/test.manifest.js
+++ b/tests/unit/test-util/test.manifest.js
@@ -70,7 +70,7 @@ describe('util/manifest', () => {
               );
               assert.include(
                 error.message,
-                'Unexpected token "," (0x2C) in JSON at position 51'
+                'Expected double-quoted property name in JSON at position 51'
               );
               assert.include(error.message, manifestFile);
             })

[Rob--W]

We could/should check the Node version in the test and choose the error message based on that (e.g by detecting the version with process.versions.node).

Another alternative is to accept either error message, regardless of node version, e.g. /Unexpected string in JSON at position 14|Expected ':' after property name in JSON at position 14/

[yan12125]

accept either error message, regardless of node version

It sounds simpler. I can create a pull request if you wish.

[Rob--W]

Either approach works, but the advantage of using process.versions is that it's very clear which versions are targeted. Here is an example:
https://github.com/Rob--W/cors-anywhere/blob/70aaa22b3f9ad30c8566024bf25484fd1ed9bda9/test/test.js#L428-L448

Using a single regexp with /err1|err2/ would also work, but the disadvantage of that is that it is not immediately obvious what the message is supposed to be, and that if the error is always err1, that there would not be any errors even if the expectation contains a never-seen error (i.e. err2 in /err1|err2/). The confusion over the error message could easily be addressed by including a comment to match the error message with Node version. If we ever decide to drop support for older Node versions, that comment can be relied upon.

So to summarize, patches are welcome, and either approach is fine :)