build: Update syncpack #24005
build: Update syncpack #24005
Conversation
github-actions
bot
added
base: main
There was a problem hiding this comment.
PR Overview
This PR updates syncpack from v9 to v13 to address a dependabot alert and adjust configuration and telemetry tracking in the FluidFramework codebase.
- Updated syncpack configuration files to reflect changes in dependency handling for v13.
- Refactored signal telemetry tracking in the container runtime, including renaming methods and introducing a new interface.
- Removed an obsolete interface from connectionTelemetry.
Reviewed Changes
| File | Description |
|---|---|
| build-tools/syncpack.config.cjs | Updated configuration blocks for dependency ranges and ignored entries. |
| packages/runtime/container-runtime/src/signalTelemetryProcessing.ts | Refactored signal telemetry tracking with new function names and interface. |
| packages/runtime/container-runtime/src/containerRuntime.ts | Updated signal tracking method calls to conditionally track broadcast signals. |
| syncpack.config.cjs | Introduced a new configuration block for dev dependencies with 'previous' packages. |
| packages/runtime/container-runtime/src/connectionTelemetry.ts | Removed the unused IPerfSignalReport interface. |
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
alexvy86
force-pushed
the
update-syncpack
branch
from
324d92b to
6e21632
Compare
Co-authored-by: Copilot <[email protected]>
| @@ -105,6 +121,7 @@ module.exports = { | |||
| "@fluid-tools/version-tools", | |||
| "@fluidframework/build-tools", | |||
| "@fluidframework/bundle-size-tools", | |||
| "@fluidframework/build-tools-bin", | |||
There was a problem hiding this comment.
This is how packages in the build-tools release group reference an older version of build-tools for their own builds.
| @@ -7,7 +7,7 @@ parameters: | |||
| # The node version required. The version must already be installed in the image. | |||
| - name: version | |||
| type: string | |||
| default: '18.17.1' | |||
| default: '18.20.7' | |||
There was a problem hiding this comment.
Syncpack 13 wants Node >=18.18.0. Might as well go to the latest. Not sure how long it'll take to complete the move to Node20 so this still seems worth doing. And I can update our build agent images to also have 18.20.7 baked in.
There was a problem hiding this comment.
Is this really how simple it is to use a new version? I asked repeatedly that it be made this simple and was told NOT POSSIBLE. It's really this easy?
There was a problem hiding this comment.
AFAIK yes, for the purpose of the pipelines using it when they run. I can't remember who added the option, I would have guessed you 😆 but maybe Andrei. Locally I think we'd want to update the engines in package.json, like I noticed you did in #22146 and I didn't here.
There was a problem hiding this comment.
It's also what @CraigMacomber did in #21965 to see how painful the move to Node20 would be.
There was a problem hiding this comment.
<sigh> I'm just frustrated because I did this upgrade months ago but couldn't proceed because CI was running an old node version, despite me making this same change in my PR. I even split the node upgrade out to a separate change, but I was so annoyed by the discussion that I eventually abandoned the PR. It's not clear to me why this didn't work 6 months ago, I guess.
Description
Updates syncpack from v9 to v13 to address this dependabot alert. I did the update major-by-major and had to apply some changes to the syncpack config files (one of them required for v10 apparently isn't necessary anymore with v13). Also, had to skip v12 because it started requiring NodeJS 20 due to a dependency, but they later fixed that on v13.
Reviewer Guidance
The review process is outlined on this wiki page.