-
Notifications
You must be signed in to change notification settings - Fork 91
ci: don't install fixture's deps if next version requierment is not satisfied #3103
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
Merged
pieh
merged 17 commits into
main
from
ci/dont-install-fixture-deps-if-next-version-is-not-satisfied
Sep 6, 2025
Merged
ci: don't install fixture's deps if next version requierment is not satisfied #3103
pieh
merged 17 commits into
main
from
ci/dont-install-fixture-deps-if-next-version-is-not-satisfied
Sep 6, 2025
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
📊 Package size report No changes
Unchanged files
🤖 This report was automatically generated by pkg-size-action |
@pieh
pieh
force-pushed
the
ci/dont-install-fixture-deps-if-next-version-is-not-satisfied
branch
from
September 4, 2025 16:55
0ad1257 to
6160b03
Compare
...re-deps-if-next-version-is-not-satisfied
pieh
pieh
commented
Sep 5, 2025
Comment on lines
+84
to
+103
const addPrefix = () => {
let isFirstChunk = true
return new Transform({
transform(chunk, encoding, callback) {
if (isFirstChunk) {
this.push(`[${fixture}] `)
isFirstChunk = false
}
this.push(chunk.toString().replace(/\n/gm, `\n[${fixture}] `))
callback()
},
flush(callback) {
// final transform might create non-terminated line with a prefix
// so this is just to make sure we end with a newline so further writes
// to same destination stream start on a new line for better readability
this.push('\n')
callback()
},
})
}
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.
note - previous setup with reusing same transform was resulting in only seeing output from 1 .pipe() - so instead for each .pipe we create clone of transform (this was noticed when I used DEBUG condition to print out npm/pnpm list next that output was silent for that)
pieh
pieh
commented
Sep 5, 2025
Comment on lines
+116
to
124
if (process.env.DEBUG) {
const npmListPromise = execaCommand(
packageManager?.startsWith('pnpm') ? 'pnpm list next' : 'npm list next',
{ cwd, stdio: 'pipe', reject: false },
)
npmListPromise.stdout?.pipe(addPrefix()).pipe(process.stdout)
npmListPromise.stderr?.pipe(addPrefix()).pipe(process.stderr)
await npmListPromise
}
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 was useful in debugging, so I think I'd like to leave this here (behind DEBUG toggle to avoid it being too spammy in regular usage)
pieh
pieh
commented
Sep 5, 2025
Comment on lines
-63
to
-66
if (NEXT_VERSION !== 'latest') {
await setNextVersionInFixture(cwd, NEXT_VERSION, {
logPrefix: `[${fixture}] `,
})
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.
Note - the similar check is now done inside setNextVersionInFixture to avoid doing any actual changes for latest, but still allow to figure out if NEXT_VERSION satisfies potential fixture constraints like those:
opennextjs-netlify/tests/fixtures/ppr/package.json
Lines 15 to 19 in 5e28132
"test": {
"dependencies": {
"next": "canary"
}
}
opennextjs-netlify/tests/fixtures/middleware-node-runtime-specific/package.json
Lines 15 to 19 in 5e28132
"test": {
"dependencies": {
"next": ">=15.2.0"
}
}
Note - those are not "dependencies" - those our own definitions that control which next versions are applicable for this fixture (for example for node middleware fixtures, we only allow next versions that actually support that, etc)
pieh
pieh
commented
Sep 5, 2025
!satisfies(checkVersion, versionConstraint, { includePrerelease: true }) &&
!(versionConstraint === 'canary'
? isNextCanary()
: satisfies(checkVersion, versionConstraint, { includePrerelease: 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 is not ideal, but as mentioned few lines above (Line 103) - we can't really use semver's satisfies when a tag (like canary) is used, so this is alternative solution for that
mrstork
mrstork
reviewed
Sep 5, 2025
- name: 'Install Node'
uses: actions/setup-node@v4
with:
node-version: '20.x'
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.
I'm not sure if we want a full revert of the last change, but I also updated node-version in a few other files. We likely don't need the same logic everywhere, but we might want to go back to 18 for some of them
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.
The other workflows are (that I didn't touch and were updated in https://github.com/opennextjs/opennextjs-netlify/pull/3101/files to use node 20):
- deno unit tests (I think we use node/npm just to install deps, but otherwise use deno)
- size check
- lint
I don't think it really matter which node version we use there, so might as well keep them on node 20
mrstork
mrstork
approved these changes
Sep 5, 2025
...-is-not-satisfied
@pieh
pieh
deleted the
ci/dont-install-fixture-deps-if-next-version-is-not-satisfied
branch
September 6, 2025 12:48
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
Description
We are running tests on node 18 right now. Next@16 will be bumping minimal node version to 20.9 and will be failing builds if it's not met.
We do have some fixtures that require next@canary (which already require node 20.9) and if the next version constraint is not met for the fixture - we do install and build fixture, just with whatever is in package.json instead of version we ask to test.
This skips the installation and build for fixtures that do not meet next version constraints so that we could continue to test with node 18 on next@<16
Documentation
Tests
Not really adding tests, but I did run:
I think they all work as expected now:
There are some failures there, but I think it's normal flakiness, I didn't want to restart them because it's the setup I wanted to assert on, not results on the tests themselves
Relevant links (GitHub issues, etc.) or a picture of cute animal