Updates ci-cd commands to set node version based on SPFx project version. Closes #6717 #6766
Conversation
There was a problem hiding this comment.
Really cool enhancement @Adam-it! This will improve the command a lot.
However, while reviewing, I found a few things we should take a look at first.
| const nodeVersion: string = spfx.getHighestNodeVersion(versionRequirements.node.range); | ||
|
|
||
| if (nodeVersion) { | ||
| this.assignNodeVersion(workflow, nodeVersion); |
There was a problem hiding this comment.
Is it worth it to create an assignNodeVersion function for this if it's only used once? Other options below that are injected into the workflow don't have this either.
There was a problem hiding this comment.
Reusability is one thing sure, but here I think I was aiming for readability (I think as I did this change so long ago it is impossible to remember why I did it this way 🤣). Most probably this was more to extract the logic that manipulates the json to some helper method with a more meaningful name instead of the code itself.
That way when someone will read the main logic of updating the workflow file it should be super clear what is going on in the flow without the need to actually understand/reason over each line that modifies the workflow file structure.
If you insist I may remove and just add this line here, but I think it is just cleaner this way.
src/m365/spfx/commands/project/project-github-workflow-add.spec.ts
Outdated
Show resolved
Hide resolved
src/m365/spfx/commands/project/project-github-workflow-add.spec.ts
Outdated
Show resolved
Hide resolved
|
|
||
| const versions = ranges.map(range => { | ||
| if (range.includes('<')) { | ||
| const upperBound = range.split('<')[1].trim().split(' ')[0]; |
There was a problem hiding this comment.
At this line, we expect that there is a space between the < sign and the version number, which shouldn't be the case.
| typeof project.packageJson?.dependencies?.['knockout'] !== 'undefined'; | ||
| }, | ||
|
|
||
| getHighestNodeVersion(versionRange: string): string { |
There was a problem hiding this comment.
I wonder if we're not reinventing the wheel with this function. We have the semver package installed in our project to make it easier to play with semantic versioning.
For example, the value >=16.13.0 <17.0.0 || >=18.17.1 <19.0.0, can't we split it on || and take the last part. Then use semver.minVersion(...) to get the lowest version.
After that, we can return minVersion.major + '.x' which will result in 18.x.
Since GitHub and DevOps will always take the latest version of v18, it will never be lower than 18.17.1.
Just dropping suggestions, I think we can replace quite some of the logic with the semver package.
|
|
||
| it('returns correct Node version for a given range', () => { | ||
| const version = spfx.getHighestNodeVersion('>=14.0.0 <15.0.0 || >=16.0.0 <17.0.0'); | ||
| assert.strictEqual(version, '17.0.x'); |
There was a problem hiding this comment.
The version should be smaller than 17, right (<17.0.0)?
So I think this test is not correct.
|
|
||
| it('returns correct Node version for a single version', () => { | ||
| const version = spfx.getHighestNodeVersion('^10'); | ||
| assert.strictEqual(version, '10.0.x'); |
There was a problem hiding this comment.
Shouldn't this result in 10.x instead?
There was a problem hiding this comment.
Pull Request Overview
This pull request updates CI/CD workflow commands to dynamically determine the Node.js version based on the SharePoint Framework (SPFx) project version rather than using a hardcoded version. The PR refactors shared compatibility data into a separate module and adds logic to compute the appropriate Node.js version from version ranges.
- Extracts SPFx version compatibility matrix to a shared module
- Adds utility function to parse version ranges and determine highest compatible Node.js version
- Updates GitHub workflow and Azure DevOps pipeline commands to set Node.js version dynamically
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/spfx.ts | Added utility function to extract highest Node version from semver range |
| src/utils/spfx.spec.ts | Added test coverage for the new utility function |
| src/m365/spfx/commands/spfx-doctor.ts | Removed duplicate compatibility matrix, now imports from shared module |
| src/m365/spfx/commands/project/project-github-workflow-add.ts | Added logic to determine and set Node version based on SPFx project version |
| src/m365/spfx/commands/project/project-github-workflow-add.spec.ts | Added test coverage for version determination and error handling |
| src/m365/spfx/commands/project/project-azuredevops-pipeline-add.ts | Added logic to determine and set Node version based on SPFx project version |
| src/m365/spfx/commands/project/project-azuredevops-pipeline-add.spec.ts | Added test coverage for version determination and error handling |
| src/m365/spfx/commands/project/DeployWorkflow.ts | Changed default Node version from hardcoded "22.x" to empty string |
| src/m365/spfx/commands/SpfxCompatibilityMatrix.ts | New shared module containing SPFx compatibility matrix data |
|
|
||
| it('returns correct Node version for a given range', () => { | ||
| const version = spfx.getHighestNodeVersion('>=14.0.0 <15.0.0 || >=16.0.0 <17.0.0'); | ||
| assert.strictEqual(version, '17.0.x'); |
There was a problem hiding this comment.
The expected version should be '16.0.x', not '17.0.x'. The range '>=16.0.0 <17.0.0' allows versions from 16.0.0 up to (but not including) 17.0.0, so the highest compatible version in this range is 16.x.
| assert.strictEqual(version, '17.0.x'); | |
| assert.strictEqual(version, '16.0.x'); |
| const upperBound = range.split('<')[1].trim().split(' ')[0]; | ||
| const parts = upperBound.split('.'); | ||
| return `${parts[0]}.${parts[1]}`; |
There was a problem hiding this comment.
When handling ranges with '<', this logic extracts the upper bound but does not adjust it to represent the highest compatible version. For example, with '<17.0.0', this returns '17.0' when it should return '16.0' (the highest major.minor below the upper bound). Consider decrementing the major or minor version when the upper bound uses a '<' operator.
Adam-it
force-pushed
the
updates-ci-cd-commands-to-support-all-spfx-versions
branch
from
47d9054 to
d6cbe35
Compare
|
@milanholemans I added the first batch of fixes to your comments that I could do without thinking 😁. |
2 participants
Closes #6717