ci: don't install fixture's deps if next version requierment is not satisfied #3103
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
Contributor
📊 Package size report No changes
Unchanged files
🤖 This report was automatically generated by pkg-size-action |
pieh
force-pushed
the
ci/dont-install-fixture-deps-if-next-version-is-not-satisfied
branch
from
0ad1257 to
6160b03
Compare
…re-deps-if-next-version-is-not-satisfied
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() | ||
| }, | ||
| }) | ||
| } |
Contributor
Author
There was a problem hiding this comment.
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
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 | ||
| } |
Contributor
Author
There was a problem hiding this comment.
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
commented
Sep 5, 2025
Comment on lines
-63
to
-66
| if (NEXT_VERSION !== 'latest') { | ||
| await setNextVersionInFixture(cwd, NEXT_VERSION, { | ||
| logPrefix: `[${fixture}] `, | ||
| }) |
Contributor
Author
There was a problem hiding this comment.
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
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
commented
Sep 5, 2025
| !satisfies(checkVersion, versionConstraint, { includePrerelease: true }) && | ||
| !(versionConstraint === 'canary' | ||
| ? isNextCanary() | ||
| : satisfies(checkVersion, versionConstraint, { includePrerelease: true })) && |
Contributor
Author
There was a problem hiding this comment.
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
reviewed
Sep 5, 2025
| - name: 'Install Node' | ||
| uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: '20.x' |
Contributor
There was a problem hiding this comment.
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
Contributor
Author
There was a problem hiding this comment.
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
approved these changes
Sep 5, 2025
…-is-not-satisfied
pieh
deleted the
ci/dont-install-fixture-deps-if-next-version-is-not-satisfied
branch
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.
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