chore: move to ESM, update all dependencies #113

Trott · 2023-03-07T01:04:55Z

chalk requires ESM. Sure, let's do it.

nopt doesn't support Node.js 12.x so this is a breaking change to drop 12.x support. (I guess the ESM stuff might also be a breaking change for people using this as a dependency rather than a CLI. Does anyone do that?)

Trott · 2023-03-28T05:54:38Z

@nodejs/tsc Anyone willing to review this?

.github/workflows/node.js.yml

bin/cmd.mjs

lib/tap.mjs

lib/utils.mjs

test/cli-test.mjs

test/utils-test.mjs

test/validator.mjs

lib/validator.mjs

Trott · 2023-03-28T13:58:36Z

The coverage report in tests is now broken. Maybe tap doesn't support ESM coverage? (I haven't looked. I'm just guessing.) Replacing with c8 should work if all else fails. Can do that in a subsequent PR. Or I can do it in this PR if people are confident it's the right way to go. Coverage has been advisory-only anyway.

Old output:

> tap --no-check-coverage --coverage-report=text-summary


=============================== Coverage summary ===============================
Statements   : 92.79% ( 451/486 )
Branches     : 76.01% ( 187/246 )
Functions    : 94.2% ( 65/69 )
Lines        : 93.93% ( 434/462 )
================================================================================

Output with c8:

-------------------|---------|----------|---------|---------|-------------------
File               | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
-------------------|---------|----------|---------|---------|-------------------
All files          |    94.7 |    87.71 |   94.11 |    94.7 |                   
 ...alidate-commit |     100 |      100 |     100 |     100 |                   
  index.mjs        |     100 |      100 |     100 |     100 |                   
 ...ate-commit/bin |   91.92 |    81.81 |     100 |   91.92 |                   
  cmd.mjs          |   91.92 |    81.81 |     100 |   91.92 | 86-87,100-110     
 ...ate-commit/lib |   88.91 |       80 |   91.66 |   88.91 |                   
  ...at-pretty.mjs |    87.2 |    52.38 |     100 |    87.2 | ...33-34,43-44,63 
  format-tap.mjs   |    58.1 |    61.53 |      60 |    58.1 | ...30,34-48,63-74 
  rule.mjs         |     100 |      100 |     100 |     100 |                   
  tap.mjs          |   94.06 |    94.11 |   93.33 |   94.06 | 65-66,70-74       
  utils.mjs        |     100 |    90.47 |     100 |     100 | 47-48             
  validator.mjs    |   98.01 |    90.47 |     100 |   98.01 | 90-91             
 ...mmit/lib/rules |   99.26 |    97.87 |     100 |   99.26 |                   
  ...s-trailer.mjs |     100 |      100 |     100 |     100 |                   
  fixes-url.mjs    |   94.79 |    93.33 |     100 |   94.79 | 91-95             
  ...ter-title.mjs |     100 |       80 |     100 |     100 | 17                
  line-length.mjs  |     100 |      100 |     100 |     100 |                   
  metadata-end.mjs |     100 |      100 |     100 |     100 |                   
  pr-url.mjs       |     100 |      100 |     100 |     100 |                   
  reviewers.mjs    |     100 |      100 |     100 |     100 |                   
  subsystem.mjs    |     100 |      100 |     100 |     100 |                   
  title-format.mjs |     100 |      100 |     100 |     100 |                   
  title-length.mjs |     100 |      100 |     100 |     100 |                   
-------------------|---------|----------|---------|---------|-------------------

mcollina

lgtm

nopt@7 drops support for Node.js 12.x so we'll drop support too. BREAKING CHANGE: drop support for Node.js 12.x

Co-authored-by: Michaël Zasso <targos@protonmail.com>

tniessen · 2023-03-29T16:56:21Z

@Trott is there a reason to use .mjs instead of setting the module type to ESM in package.json?

Trott · 2023-03-29T17:07:45Z

@Trott is there a reason to use .mjs instead of setting the module type to ESM in package.json?

Personal preference. I prefer .mjs because it is very hard to miss, as opposed to setting things in package.json which seems more magical and hiding things to me.

But also: I know I'm in the minority on this, so I'll update it to set things in package.json.

aduh95 · 2023-03-29T17:08:53Z

breaking change for people using this as a dependency rather than a CLI. Does anyone do that?

node-core-utils does that (and that's the package listed in https://www.npmjs.com/package/core-validate-commit?activeTab=dependents FWIW).

aduh95

Should we drop official support for v14.x at the same time? v14.x EOL is quite close, and that would allow us to switch to node:test for the tests in a future PR.

I'm with you (Rich) for .mjs, but no strong feelings.

richardlau · 2023-03-29T17:12:22Z

breaking change for people using this as a dependency rather than a CLI. Does anyone do that?

node-core-utils does that (and that's the package listed in https://www.npmjs.com/package/core-validate-commit?activeTab=dependents FWIW).

I thought it did too, but node-core-utils executes core-validate-commit as a CLI tool: https://github.com/nodejs/node-core-utils/blob/cc4b11c3de20b0c4b28e9973794b51313a7ed766/lib/landing_session.js#L427-L434

Trott · 2023-03-29T17:25:17Z

Should we drop official support for v14.x at the same time? v14.x EOL is quite close, and that would allow us to switch to node:test for the tests in a future PR.

Can't drop 14.x until nodejs/build#3214 is resolved.

GeoffreyBooth · 2023-03-29T17:30:21Z

But also: I know I’m in the minority on this, so I’ll update it to set things in package.json.

I think in this case specifically, it’s better to keep the .js extensions on all the files for more legible git history. Yes it seems like git is able to track all the changes across the renames, but why add that complexity? Simply avoiding an unnecessary rename seems safer.

Trott · 2023-03-29T17:33:56Z

I think in this case specifically, it’s better to keep the .js extensions on all the files for more legible git history. Yes it seems like git is able to track all the changes across the renames, but why add that complexity? Simply avoiding an unnecessary rename seems safer.

Prioritizing git history over code clarity seems like the wrong trade-off to me, but views on this sort of thing tend to depend on assumptions, speculation, and personal preference. So I've already conceded and named the files back to .js.

GeoffreyBooth · 2023-03-29T17:40:18Z

bin/cmd.js

-const utils = require('../lib/utils')
-const subsystem = require('../lib/rules/subsystem')
+import { exec } from 'node:child_process'
+import fs from 'node:fs'


Nit: All of these could be import fs from 'node:fs/promises'

GeoffreyBooth · 2023-03-29T17:41:23Z

package.json

@@ -3,17 +3,17 @@
  "version": "3.20.0",
  "description": "Validate the commit message for a particular commit in node core",
  "main": "index.js",


Suggested change

"main": "index.js",

"exports": "index.js",

We probably want to also export the package.json file.

GeoffreyBooth · 2023-03-29T17:41:44Z

package.json

@@ -3,17 +3,17 @@
  "version": "3.20.0",


This should probably be a major version bump?

We do the version bumps when we release.

Trott force-pushed the new-version branch from c17b355 to 80c6331 Compare March 28, 2023 05:50

targos approved these changes Mar 28, 2023

View reviewed changes

mcollina approved these changes Mar 29, 2023

View reviewed changes

Trott and others added 23 commits March 29, 2023 09:45

chore: update nopt from 6.x to 7.0.0

620dd8a

nopt@7 drops support for Node.js 12.x so we'll drop support too. BREAKING CHANGE: drop support for Node.js 12.x

chore: switch to ESM in preparation for chalk update

4606222

chore: update chalk 4.x to 5.2.0

7698d02

Update .github/workflows/node.js.yml

8ffbae1

Co-authored-by: Michaël Zasso <targos@protonmail.com>

Update bin/cmd.mjs

b7ca85d

Co-authored-by: Michaël Zasso <targos@protonmail.com>

Update lib/tap.mjs

33d1f5f

Co-authored-by: Michaël Zasso <targos@protonmail.com>

Update lib/utils.mjs

2257d18

Co-authored-by: Michaël Zasso <targos@protonmail.com>

Update lib/utils.mjs

b78ff95

Co-authored-by: Michaël Zasso <targos@protonmail.com>

Update lib/utils.mjs

d6acf92

Co-authored-by: Michaël Zasso <targos@protonmail.com>

Update lib/utils.mjs

4db9afd

Co-authored-by: Michaël Zasso <targos@protonmail.com>

Update lib/utils.mjs

c02cb1f

Co-authored-by: Michaël Zasso <targos@protonmail.com>

Update bin/cmd.mjs

9e3c5ca

Co-authored-by: Michaël Zasso <targos@protonmail.com>

Update lib/format-pretty.mjs

9259c55

Co-authored-by: Michaël Zasso <targos@protonmail.com>

Update lib/utils.mjs

140cc69

Co-authored-by: Michaël Zasso <targos@protonmail.com>

Update test/utils-test.mjs

7f775f7

Co-authored-by: Michaël Zasso <targos@protonmail.com>

Update lib/validator.mjs

d5d5a96

Co-authored-by: Michaël Zasso <targos@protonmail.com>

Update test/cli-test.mjs

8bf157d

Co-authored-by: Michaël Zasso <targos@protonmail.com>

Update test/validator.mjs

90c1925

Co-authored-by: Michaël Zasso <targos@protonmail.com>

Update test/cli-test.mjs

6b7a99d

Co-authored-by: Michaël Zasso <targos@protonmail.com>

Update test/cli-test.mjs

feb9c5b

Co-authored-by: Michaël Zasso <targos@protonmail.com>

fixup! Update test/cli-test.mjs

c118d88

Update test/validator.mjs

47a6f05

Co-authored-by: Michaël Zasso <targos@protonmail.com>

fixup! Update test/validator.mjs

22dace2

aduh95 approved these changes Mar 29, 2023

View reviewed changes

fixup: use package.json type field rather than .mjs file extension

357ddcf

Trott force-pushed the new-version branch from c56c50b to 357ddcf Compare March 29, 2023 17:19

GeoffreyBooth reviewed Mar 29, 2023

View reviewed changes

Trott merged commit 8cf9e86 into nodejs:main Mar 30, 2023
4 checks passed

Trott deleted the new-version branch March 30, 2023 15:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: move to ESM, update all dependencies #113

chore: move to ESM, update all dependencies #113

Trott commented Mar 7, 2023 •

edited

Trott commented Mar 28, 2023

Trott commented Mar 28, 2023

mcollina left a comment

tniessen commented Mar 29, 2023

Trott commented Mar 29, 2023

aduh95 commented Mar 29, 2023

aduh95 left a comment •

edited

richardlau commented Mar 29, 2023

Trott commented Mar 29, 2023

GeoffreyBooth commented Mar 29, 2023

Trott commented Mar 29, 2023

GeoffreyBooth Mar 29, 2023

GeoffreyBooth Mar 29, 2023

aduh95 Mar 29, 2023

GeoffreyBooth Mar 29, 2023

Trott Mar 30, 2023

Trott Mar 30, 2023

chore: move to ESM, update all dependencies #113

chore: move to ESM, update all dependencies #113

Conversation

Trott commented Mar 7, 2023 • edited

Trott commented Mar 28, 2023

Trott commented Mar 28, 2023

mcollina left a comment

Choose a reason for hiding this comment

tniessen commented Mar 29, 2023

Trott commented Mar 29, 2023

aduh95 commented Mar 29, 2023

aduh95 left a comment • edited

Choose a reason for hiding this comment

richardlau commented Mar 29, 2023

Trott commented Mar 29, 2023

GeoffreyBooth commented Mar 29, 2023

Trott commented Mar 29, 2023

GeoffreyBooth Mar 29, 2023

Choose a reason for hiding this comment

GeoffreyBooth Mar 29, 2023

Choose a reason for hiding this comment

aduh95 Mar 29, 2023

Choose a reason for hiding this comment

GeoffreyBooth Mar 29, 2023

Choose a reason for hiding this comment

Trott Mar 30, 2023

Choose a reason for hiding this comment

Trott Mar 30, 2023

Choose a reason for hiding this comment

Trott commented Mar 7, 2023 •

edited

aduh95 left a comment •

edited