build(client): Add syncpack to find and fix mismatched dependency versions #14042

tylerbutler · 2023-02-07T00:59:28Z

pnpm enables us to use different versions of dependencies in different projects within the same release group (workspace in pnpm terms). While there are legitimate uses for this capability, we prefer to use the same version of dependencies across the repo whenever possible.

This PR uses syncpack to lint and fix version differences and enforce dependency range types (e.g. tilde vs. caret deps). I did not add these scripts to CI; they can only be run manually using the following scripts:

syncpack:deps
syncpack:deps:fix
syncpack:versions
syncpack:versions:fix

The scripts already revealed some inconsistencies which I have fixed in this PR so you can see what it found.

A future PR will add this to CI in some way; details TBD.

Josmithr · 2023-02-07T18:07:58Z

Does lerna not enforce matching deps via the --strict flag when run under pnpm, I take it?

With regards to number 2: I don't know how well it fits into the current CI infra, but it seems like the check should run at the root of each release group on a per-group basis if possible (rather than policy-check) IMO. But I don't know how reasonable a direction that is in the short term.

Update: Remembered that the strict flag is a bootstrap argument, and we no longer run that script. I wonder if there is a lerna config option to do something comperable?

tylerbutler · 2023-02-07T22:07:37Z

Does lerna not enforce matching deps via the --strict flag when run under pnpm, I take it?

Update: Remembered that the strict flag is a bootstrap argument, and we no longer run that script. I wonder if there is a lerna config option to do something comperable?

Lerna is (appropriately, IMO) deprecating its package-management features:

Lerna has historically its own dependency management solution: lerna bootstrap. This was required because at the time when Lerna was first released, there were no native solutions available. Nowadays the modern package managers come with a built-in "workspaces" solution, so it is highly recommended to go with that instead. lerna bootstrap and other related commands will be officially deprecated in Lerna v7. See lerna/lerna#3410

Lerna also blocks bootstrap and similar commands when used with pnpm:

...block usage of bootstrap, link, and add commands. Instead, you should use pnpm commands directly to manage dependencies

Since they're getting out of the package management game altogether, and pnpm deliberately allows different versions per package, I think we have to look elsewhere. pnpm does have some good features for overriding dependencies, but it doesn't seem to fit our needs as well: https://pnpm.io/package_json#pnpmoverrides

Josmithr · 2023-02-08T18:53:52Z

Does lerna not enforce matching deps via the --strict flag when run under pnpm, I take it?
Update: Remembered that the strict flag is a bootstrap argument, and we no longer run that script. I wonder if there is a lerna config option to do something comperable?

Lerna is (appropriately, IMO) deprecating its package-management features:

Lerna has historically its own dependency management solution: lerna bootstrap. This was required because at the time when Lerna was first released, there were no native solutions available. Nowadays the modern package managers come with a built-in "workspaces" solution, so it is highly recommended to go with that instead. lerna bootstrap and other related commands will be officially deprecated in Lerna v7. See lerna/lerna#3410

Lerna also blocks bootstrap and similar commands when used with pnpm:

...block usage of bootstrap, link, and add commands. Instead, you should use pnpm commands directly to manage dependencies

Since they're getting out of the package management game altogether, and pnpm deliberately allows different versions per package, I think we have to look elsewhere. pnpm does have some good features for overriding dependencies, but it doesn't seem to fit our needs as well: https://pnpm.io/package_json#pnpmoverrides

100% agree with Lerna getting out of package management. And I agree that the pnpm feature doesn't look like the right answer. Linting seems like a reasonable choice to me. And it looks like syncpack supports both pnpm and yarn, so there shouldn't be an issue if/when we migrate.

Abe27342 · 2023-02-08T19:58:08Z

I agree with previous feedback that linting is a reasonable approach. It definitely seems a lot cleaner than hooking into pnpm behavior.

JamieMason · 2023-02-15T08:07:32Z

syncpack.config.cjs

+				"webpack-dev-server",
+			],
+			packages: ["**"],
+			range: "~",


FYI this line won't do anything since this entry is under versionGroups and not semverGroups, you might actually be able to remove this versionGroup and the behaviour will be the same as far as I can tell. Shout if I can help with anything confusing about syncpack 👍

Hi @JamieMason, thanks for stopping by! 👋🏻 And thanks for writing/maintaining a great infrastructure tool!

this line won't do anything since this entry is under versionGroups and not semverGroups

Thank you, I missed that! I removed this and also started using the new custom types feature. Very useful! I may have found a bug which I'll point out in a separate comment.

Shout if I can help with anything confusing about syncpack

Is my understanding of the groups terminal output correct? Namely that they're numbered, but that the numeric order is the inverse of the array order in the config? When building the syncpack config, I found it very confusing to map the output to the group/rule (I think of them as "rules", like linting). Because a given package/dep can only fit into one group, it was difficult to debug why a given combo winds up in a group I didn't expect.

When you don't know what the state of your repo is -- what packages have mismatched deps that you want to ignore, etc. -- then it helps tremendously if it's easy to map syncpack's output -- this package has this dep that is mismatched -- to the "rule" that is being applied. A feature that may help is to add an optional identifier/name field to the semverGroups and versionGroups, and use those names in the terminal output/logs.

One minor note: we use an unusual semver range for our in-workspace dependencies. For example, ">=2.0.0-internal.3.0.0 <2.0.0-internal.3.1.0". Unless I ignore these dependencies with an ignored versionGroup, syncpack overrwrites the versions with "". Maybe a bug, but could also be expected/undefined behavior since the version range is unusual.

Is my understanding of the groups terminal output correct? Namely that they're numbered, but that the numeric order is the inverse of the array order in the config? When building the syncpack config, I found it very confusing to map the output to the group/rule (I think of them as "rules", like linting). Because a given package/dep can only fit into one group, it was difficult to debug why a given combo winds up in a group I didn't expect.

When you don't know what the state of your repo is -- what packages have mismatched deps that you want to ignore, etc. -- then it helps tremendously if it's easy to map syncpack's output -- this package has this dep that is mismatched -- to the "rule" that is being applied.

Yeah 100% agree, it's weird how it is at the moment – I've created JamieMason/syncpack#120 to update that.

A feature that may help is to add an optional identifier/name field to the semverGroups and versionGroups, and use those names in the terminal output/logs.

Good idea, I've created JamieMason/syncpack#118 for that.

One minor note: we use an unusual semver range for our in-workspace dependencies. For example, ">=2.0.0-internal.3.0.0 <2.0.0-internal.3.1.0". Unless I ignore these dependencies with an ignored versionGroup, syncpack overrwrites the versions with "". Maybe a bug, but could also be expected/undefined behavior since the version range is unusual.

That sounds like a bug, I've created JamieMason/syncpack#119.

Thanks a lot for this feedback @tylerbutler, lots of good stuff has come out of it.

tylerbutler · 2023-02-15T18:10:03Z

syncpack.config.cjs

+		// engines.npm should always use ^ range
+		{
+			dependencyTypes: ["enginesNpm"],
+			dependencies: ["**"],
+			packages: ["**"],
+			range: "^",
+		},


@JamieMason This is the possible bug in custom types. I've defined engines.node and engines.npm custom types, but in testing I could only get one to trigger at a time. I can do more testing and give more details if it's helpful. I'm also happy to file a proper issue. Thanks for taking the time to help!

This is probably a bug, I'll take a look 👍

I tried to reproduce this in this commit but the tests I expected to fail are actually passing – could you create an issue with anything more you can think of? thanks!

I did a bit more testing on this and found several inconsistencies. I wrote them up a readme on my branch along with how to reproduce them with our config. Details are at https://github.com/tylerbutler/FluidFramework/tree/syncpack-issues-repro

hey @tylerbutler, I wasn't sure where best to reply but I've put some comments to what's in the readme in this gist

EDIT: Some of these issues have since been fixed in https://github.com/JamieMason/syncpack/releases/tag/9.7.4

@JamieMason Thanks for looking into those issues and resolving them so quickly! I definitely had some config bugs on my end so thanks for looking over them. I updated to 9.8.4 and verified that all the issues I noticed were fixed. And the new group headers are extremely helpful for me! Cheers!

tylerbutler · 2023-02-15T18:38:23Z

I did some more thinking about this and I think policy-check is a better phase of the development cycle to run this check in, at least when running locally. It could even be a policy handler, but since policy-check is designed to work on a per-file basis, it would be hacky to do so.

In CI, I think we need to split out the tasks from lint and policy so we can run them separately both for efficiency and so it's clearer in CI what's failing and how to fix it. But that's a longer conversation that I'll write up a formal proposal for.

tylerbutler · 2023-02-17T19:34:30Z

experimental/PropertyDDS/packages/property-inspector-table/package.json

+		"@fluid-experimental/property-binder": ">=2.0.0-internal.3.1.0 <2.0.0-internal.4.0.0",
+		"@fluid-experimental/property-changeset": ">=2.0.0-internal.3.2.0 <2.0.0-internal.4.0.0",
+		"@fluid-experimental/property-dds": ">=2.0.0-internal.3.2.0 <2.0.0-internal.4.0.0",
+		"@fluid-experimental/property-properties": ">=2.0.0-internal.3.2.0 <2.0.0-internal.4.0.0",
+		"@fluid-experimental/property-proxy": ">=2.0.0-internal.3.2.0 <2.0.0-internal.4.0.0",


@JamieMason This is a possible issue I missed earlier. I expected the mismatched @fluid-experimental/property-binder version to be caught, but it was not. Now that I think about it, if the version isn't understandable by syncpack, as these presumably are, then it likely just ignores it completely? I originally thought that it would be doing a string comparison. Feel free to ignore if this is working as intended.

In 9.3.2 it should tell you that it can't fix them (where before it silently ignored them) but it should know when they mismatch and tell you there's a mismatch it cannot fix. Thanks for the repro branch, I'll see what I can find on this and the other problems.

Josmithr · 2023-02-17T21:40:24Z

syncpack.config.cjs

+				"@tiny-calc/*",
+				"@graphql-codegen/cli",
+				"@graphql-codegen/typescript",
+				"@types/chrome",


I'm curious as to the motivation for pinning the @types packages here, but not their corresponding runtime packages. Don't we want them to "match" / get updated together?

I tried to follow the conventions we have in place - which may have no reasoning. Updating these deps should be done separately from this PR, and probably individually because lots of things broke when I tested it.

All the deps updates in this PR narrow the dep range, with the exception of re-resizable, which went from an exact version to a ^ But everything else went from ^ to ~ or exact versions. That's one reason I feel safer about them.

Any plans to follow up on this later? This definitely feels like something we should address. But since these are existing patterns, I have no worries about deferring it outside of this PR.

syncpack.config.cjs

tylerbutler · 2023-02-17T23:07:35Z

Thanks for the review, @Josmithr!

tylerbutler added 7 commits February 14, 2023 15:48

peer deps

58b1fe7

build(client): Enforce dependency versions

d9c1da4

lint tasks

564195c

format

7ff4393

run fixups and pnpm i

2e18776

fixes

7492f1b

use policy instead of linting

3c7568c

tylerbutler force-pushed the syncpack branch from 3715567 to 3c7568c Compare February 15, 2023 01:33

github-actions bot removed the area: dds: tree label Feb 15, 2023

revert

83b0693

github-actions bot removed the area: dds Issues related to distributed data structures label Feb 15, 2023

formatting

008e5c6

JamieMason reviewed Feb 15, 2023

View reviewed changes

updates

1f5ae7d

tylerbutler commented Feb 15, 2023

View reviewed changes

JamieMason mentioned this pull request Feb 16, 2023

Complex semver versions are being replaced with "" JamieMason/syncpack#119

Closed

tylerbutler added 2 commits February 17, 2023 09:12

update syncpack and config

29e15ec

Merge branch 'main' into syncpack

8decb64

tylerbutler changed the title ~~feedback wanted: syncpack for enforcing dependency versions~~ build(client): Add syncpack to find and fix mismatched dependency versions Feb 17, 2023

tylerbutler marked this pull request as ready for review February 17, 2023 19:26

tylerbutler requested review from msfluid-bot and a team as code owners February 17, 2023 19:26

tylerbutler commented Feb 17, 2023

View reviewed changes

fix version

beb4844

Josmithr reviewed Feb 17, 2023

View reviewed changes

syncpack.config.cjs Show resolved Hide resolved

Josmithr reviewed Feb 17, 2023

View reviewed changes

syncpack.config.cjs Show resolved Hide resolved

Josmithr approved these changes Feb 17, 2023

View reviewed changes

tylerbutler merged commit c010387 into microsoft:main Feb 17, 2023

tylerbutler deleted the syncpack branch February 17, 2023 23:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build(client): Add syncpack to find and fix mismatched dependency versions #14042

build(client): Add syncpack to find and fix mismatched dependency versions #14042

tylerbutler commented Feb 7, 2023 •

edited

Loading

Josmithr commented Feb 7, 2023 •

edited

Loading

tylerbutler commented Feb 7, 2023

Josmithr commented Feb 8, 2023

Abe27342 commented Feb 8, 2023

JamieMason Feb 15, 2023

tylerbutler Feb 15, 2023

JamieMason Feb 16, 2023

tylerbutler Feb 15, 2023

JamieMason Feb 16, 2023

JamieMason Feb 16, 2023

tylerbutler Feb 17, 2023

JamieMason Feb 18, 2023 •

edited

Loading

tylerbutler Feb 21, 2023

tylerbutler commented Feb 15, 2023

tylerbutler Feb 17, 2023

JamieMason Feb 17, 2023

Josmithr Feb 17, 2023

tylerbutler Feb 17, 2023

Josmithr Feb 17, 2023

tylerbutler commented Feb 17, 2023

build(client): Add syncpack to find and fix mismatched dependency versions #14042

build(client): Add syncpack to find and fix mismatched dependency versions #14042

Conversation

tylerbutler commented Feb 7, 2023 • edited Loading

Josmithr commented Feb 7, 2023 • edited Loading

tylerbutler commented Feb 7, 2023

Josmithr commented Feb 8, 2023

Abe27342 commented Feb 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamieMason Feb 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tylerbutler commented Feb 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tylerbutler commented Feb 17, 2023

tylerbutler commented Feb 7, 2023 •

edited

Loading

Josmithr commented Feb 7, 2023 •

edited

Loading

JamieMason Feb 18, 2023 •

edited

Loading