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

Drop Node.js v14, run tests for 20, update dependencies #1289

Merged
merged 17 commits into from Aug 13, 2023

Conversation

iiroj
Copy link
Member

@iiroj iiroj commented Apr 20, 2023

This PR updates all dependencies, including major version listr2@6.6.0 (from 5). It also introduces a breaking change by dropping support for Node.js 14 as it's approaching EOL and listr2 needs it.

What do you think, @okonet, should we merge this at the end of the month? Node.js 14 enters EOL at 2023-04-30.

@iiroj iiroj requested a review from okonet April 20, 2023 15:39
okonet
okonet previously approved these changes Apr 20, 2023
@okonet
Copy link
Collaborator

okonet commented Apr 20, 2023

Looking good but build failed

@iiroj
Copy link
Member Author

iiroj commented Apr 20, 2023

Yeah looks like the listr2 update will be more involved. It seems to work fine but is no longer using console but process.stdout.write, making tests fail that mock the Console.

@iiroj iiroj marked this pull request as ready for review May 28, 2023 06:46
@iiroj
Copy link
Member Author

iiroj commented May 28, 2023

Let's see if things work now that it's closer to 14 EOL.

@codecov
Copy link

codecov bot commented May 28, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (5cead2d) 100.00% compared to head (217c404) 100.00%.

Additional details and impacted files
@@            Coverage Diff            @@
##            master     #1289   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           26        27    +1     
  Lines          740       760   +20     
  Branches       198       199    +1     
=========================================
+ Hits           740       760   +20     
Files Changed Coverage Δ
lib/messages.js 100.00% <ø> (ø)
lib/chunkFiles.js 100.00% <100.00%> (ø)
lib/generateTasks.js 100.00% <100.00%> (ø)
lib/getRenderer.js 100.00% <100.00%> (ø)
lib/getStagedFiles.js 100.00% <100.00%> (ø)
lib/makeCmdTasks.js 100.00% <100.00%> (ø)
lib/normalizePath.js 100.00% <100.00%> (ø)
lib/resolveGitRepo.js 100.00% <100.00%> (ø)
lib/runAll.js 100.00% <100.00%> (ø)
lib/searchConfigs.js 100.00% <100.00%> (ø)
... and 2 more

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@iiroj
Copy link
Member Author

iiroj commented May 28, 2023

@okonet can you update the list of required checks to replace Node.js v14 with v20?

README.md Outdated Show resolved Hide resolved
@iiroj iiroj requested a review from okonet June 13, 2023 16:53
@okonet
Copy link
Collaborator

okonet commented Jun 14, 2023

@iiroj checks are updated.

@iiroj
Copy link
Member Author

iiroj commented Jun 26, 2023

Ping @okonet can you check again; it's still waiting for the Node.js v14 tests?

@okonet
Copy link
Collaborator

okonet commented Jun 26, 2023

Sorry I’m sick AFK. Try pushing to the branch please

@iiroj iiroj linked an issue Jun 30, 2023 that may be closed by this pull request
package.json Outdated Show resolved Hide resolved
@iiroj
Copy link
Member Author

iiroj commented Jul 14, 2023

Ping @okonet looks like this is still waiting for Node.js 14,16,18 instead of 16,18,20.

@iiroj
Copy link
Member Author

iiroj commented Jul 16, 2023

For whatever reason Windows tests started failing again...

@iiroj iiroj force-pushed the updates-2023-04-20 branch 3 times, most recently from 7dc7ae0 to c20c3bd Compare August 11, 2023 12:41
@iiroj
Copy link
Member Author

iiroj commented Aug 11, 2023

Tests are still failing on Windows Node.js 20, because of a known issue: nodejs/node#48673

@iiroj
Copy link
Member Author

iiroj commented Aug 11, 2023

What do you think, @okonet? The tests pass on Node v20.3.1 but are broken on Windows since 20.4.0 introduced a bug hitting the emoji file test. We can either:

  1. pin v20.3.1 for now and update later once it's resolved
  2. let this PR sit until it's resolved
  3. remove v20 from the list of "expected checks" until it's resolved

I would prefer option 1. but it would mean you have to update the list of expected tests at least two more times.

@iiroj
Copy link
Member Author

iiroj commented Aug 11, 2023

Actually, I will just skip the failing test for now and open a follow-up PR for re-enabling it. 👍

@iiroj
Copy link
Member Author

iiroj commented Aug 13, 2023

@okonet well, can't merge without approval:

remote: error: GH006: Protected branch update failed for refs/heads/master.
remote: error: At least 1 approving review is required by reviewers with write access.

@iiroj iiroj merged commit f895e97 into master Aug 13, 2023
17 checks passed
@iiroj iiroj deleted the updates-2023-04-20 branch August 13, 2023 16:44
@iiroj
Copy link
Member Author

iiroj commented Aug 13, 2023

Looks like feat! didn't trigger the breaking change as expected... I'll open a follow-up PR.

@iiroj
Copy link
Member Author

iiroj commented Aug 16, 2023

@okonet if you have time it might make sense to deprecate v13.3.0:

npm deprecate lint-staged@13.3.0 "This was an accidental breaking release and is the same as v14.0.0."

@iiroj iiroj linked an issue Aug 17, 2023 that may be closed by this pull request
@glensc
Copy link

glensc commented Aug 18, 2023

maybe release 13.4.0 removing the accidental breaking release?

@iiroj
Copy link
Member Author

iiroj commented Aug 18, 2023

I don't think that's necessary, and deprecation is good enough... I'm not planning on supporting the old version anyway, and releasing a hotfix for v13 would mean having to tweak the semantic-release config... something that caused this error in the first place.

We'll migrate to 🦋 changesets to make sure versioning is easier in the future: #1314

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants