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

feat!: drop node 14.16 and 14.17 #5602

Merged
merged 2 commits into from
Apr 11, 2023
Merged

feat!: drop node 14.16 and 14.17 #5602

merged 2 commits into from
Apr 11, 2023

Conversation

danez
Copy link
Contributor

@danez danez commented Mar 30, 2023

🎉 Thanks for submitting a pull request! 🎉

Summary

Drops node 14.16 and 14.17 as discussed

Fixes #5575


For us to review and ship your PR efficiently, please perform the following steps:

  • Open a bug/issue before writing your code 🧑‍💻. This ensures we can discuss the changes and get feedback from everyone that should be involved. If you`re fixing a typo or something that`s on fire 🔥 (e.g. incident related), you can skip this step.
  • Read the contribution guidelines 📖. This ensures your code follows our style guide and
    passes our tests.
  • Update or add tests (if any source code was changed or added) 🧪
  • Update or add documentation (if features were changed or added) 📝
  • Make sure the status checks below are successful ✅

A picture of a cute animal (not mandatory, but encouraged)

@github-actions
Copy link

github-actions bot commented Mar 30, 2023

📊 Benchmark results

Comparing with 9775e40

Package size: 306 MB

⬇️ 0.00% decrease vs. 9775e40

^                                                                                  302 MB  306 MB  306 MB 
│                                                                                   ┌──┐    ┌──┐    ┌──┐  
│                                                                  271 MB  271 MB   |  |    |  |    |▒▒|  
│ ─264 MB──264 MB──264 MB──264 MB──264 MB──264 MB──264 MB──264 MB───┌──┐────┌──┐────┼──┼────┼──┼────|▒▒|──
│   ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
└───┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴──>
    T-12    T-11    T-10    T-9     T-8     T-7     T-6     T-5     T-4     T-3     T-2     T-1      T    
Legend

@danez danez requested a review from lukasholzer March 30, 2023 15:28
@danez danez marked this pull request as ready for review March 30, 2023 15:28
@danez danez requested a review from a team March 30, 2023 15:28
@danez danez requested a review from a team as a code owner March 30, 2023 15:28
@danez danez self-assigned this Mar 31, 2023
lukasholzer
lukasholzer previously approved these changes Mar 31, 2023
Copy link
Collaborator

@lukasholzer lukasholzer left a comment

Choose a reason for hiding this comment

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

🥳 Yay

@lukasholzer
Copy link
Collaborator

@danez I think you need to fix the required checks to get this through

// we need something to run in case the test above is skipped.
test('dummy test', (t) => {
t.true(true)
})
Copy link
Contributor Author

@danez danez Mar 31, 2023

Choose a reason for hiding this comment

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

This test was failing on Node.js 14.18 on windows, after migrating to vitest it works. 🤷 Maybe because I also updated hugo and recreated the lockfile of the fixture.

Also the test was not run on latest node version, although it succeeds on these versions now, so removed the restriction where to run the test

import { fileURLToPath } from 'url'

import cpy from 'cpy'
import { copy } from 'fs-extra'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

cpy cannot copy symlinks, so I exchanged it for fs-extra -> copy

Copy link
Collaborator

Choose a reason for hiding this comment

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

whats about fs.cp with recursive option? I think this uses the file system copy under the hood which should copy symlinks as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only available as of Node.js 16.7.0

@@ -29,7 +30,7 @@ export const setupFixtureTests = async function (options, factory) {

beforeAll(async () => {
if (options.fixture) fixture = await Fixture.create(options.fixture)
if (options.devServer) devServer = await startDevServer({ cwd: fixture.directory })
if (options.devServer) devServer = await startDevServer({ cwd: fixture.directory, args: ['--offline'] })
Copy link
Contributor Author

@danez danez Mar 31, 2023

Choose a reason for hiding this comment

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

when we run the dev server in fixture tests we should never read settings from the API.

Copy link
Collaborator

Choose a reason for hiding this comment

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

good point!

Copy link
Collaborator

@lukasholzer lukasholzer left a comment

Choose a reason for hiding this comment

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

@danez can we update the postinstall script to not break if being installed on a unsupported engine version?

As the engine is only a warning?

Instead exit the postinstall script with a better error messaging?
CleanShot 2023-04-11 at 13 04 27

@danez
Copy link
Contributor Author

danez commented Apr 11, 2023

I guess what you want is something like this: #5244

We can try to make the postinstall script Node.js version agnostic, but this is unrelated to this PR.

lukasholzer
lukasholzer previously approved these changes Apr 11, 2023
Copy link
Collaborator

@lukasholzer lukasholzer left a comment

Choose a reason for hiding this comment

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

LGTM

@danez danez merged commit 8bcc4c9 into main Apr 11, 2023
@danez danez deleted the node14 branch April 11, 2023 16:09
@@ -6,6 +6,6 @@
},
"license": "MIT",
"devDependencies": {
"hugo-bin": "^0.101.0"
"hugo-bin": "^0.101.5"
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Not accurate node engine version range - it should be ^14.18.0 || >=16.0.0
3 participants