-
Notifications
You must be signed in to change notification settings - Fork 380
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
Conversation
📊 Benchmark resultsComparing with 9775e40 Package size: 306 MB⬇️ 0.00% decrease vs. 9775e40
Legend
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🥳 Yay
@danez I think you need to fix the required checks to get this through |
536e693
to
7e7485d
Compare
// we need something to run in case the test above is skipped. | ||
test('dummy test', (t) => { | ||
t.true(true) | ||
}) |
There was a problem hiding this comment.
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' |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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'] }) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point!
There was a problem hiding this 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?
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -6,6 +6,6 @@ | |||
}, | |||
"license": "MIT", | |||
"devDependencies": { | |||
"hugo-bin": "^0.101.0" | |||
"hugo-bin": "^0.101.5" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@danez I think you could also go with https://github.com/fenneclab/hugo-bin/releases/tag/v0.102.0 :)
🎉 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:
passes our tests.
A picture of a cute animal (not mandatory, but encouraged)