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

Migrate to Node 20 (wip) #93

Closed
wants to merge 3 commits into from
Closed

Migrate to Node 20 (wip) #93

wants to merge 3 commits into from

Conversation

ayushmanchhabra
Copy link
Contributor

@ayushmanchhabra ayushmanchhabra commented Jun 22, 2024

Fixes: #83

"fs-extra": "8.1.0",
"get-windows-shortcut-properties": "^1.3.0",
"jest": "24.9.0",
"jest": "29.7.0",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jest > 24.9.0 is still broken on Windows (has been for 5 years). So we should not update it. I've replaced Jest with Vitest in all the repos where I need unit testing.

@ayushmanchhabra
Copy link
Contributor Author

ayushmanchhabra commented Jun 22, 2024

Closing in favour of #92 (was trying to see how to approach the issues)

"Pinned fs-extra to 8.1.0 because v9.0.0 dropped support for older node versions and we only use it for CI E2E testing",
"Pinned jest to 24.9.0. 25.1.0 is broken on Windows. Waiting for issue #9459 to be resolved",
"Pinned path-type to 4.0.0 because 5+ requires ESM import"
],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code in this repo was specifically written in a way to be as backwards compatible as possible. It currently supports Node 4.0.0 and above. The only parts of the repo that need a newer Node.js version are the linting/testing tooling. I wanted to ensure code changes wouldn't break on older Node versions, so I added Node 8.3.0 to the test runners to ensure older versions of Node are still supported. Most tooling can't run on versions of Node below 8.3.0, so that's as low as I could go.

I think we can continue to have this level of automated support, with broad Node version backward compatibility, if we continue to pin these dependencies.

Then we can extend automated support forward to Node 22, if we replace Jest with Vitest.

Alternatively we could go in the direction you have put forth in this PR, which would be one of dropping support for older Node versions. But this would then require much more manual testing to verify what versions of Node it works on, on each OS. This would be a major breaking change to the repo, and would probably require a lot of documentation updates on top of the manual testing.

If that's the direction we go in, then I'd want a 1.x.x branch created to continue Node 4+ support on, in case users need ancient Node support, but still want newer features of the library backported. Such as the Linux improvements mentioned in #76.

Paths:

  • Continue with broad Node support (least amount of effort):
    • Replace Jest with Vitest
    • Update Node from 18 to 22
  • Drop support for older Node versions
    • Replace Jest with Vitest
    • Update Node from 18 to 22
    • Need to decide what versions will be supported in the 2.x.x line
      • Light touch, still support back to Node 8 or 12 or something
      • Heavy handed, Only support 18+, re-write all code as ESM
    • Would need to validate OS support manually
    • Need to update documentation to convey this breaking change
    • Need to create 1.x.x branch

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

Successfully merging this pull request may close these issues.

Update to Node 20 (after switching to Vitest)
2 participants