Skip to content
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

use syncpack for dependency management #957

Merged
merged 2 commits into from
Apr 22, 2024
Merged

Conversation

turbocrime
Copy link
Contributor

dependencies within the monorepo must be transformed for publication.

one component of this is conversion of workspace:* version specs to something that makes sense globally - this is handled automatically by pnpm.

but many of our monorepo packages use dependencies implicit from the top level. these dependencies remain unnamed when we pack for publication. if a consumer of the package fails to install the appropriate dependency another way, they will encounter build failures, runtime errors, obscure type conflicts, or some combination of the three.

the only solution to this is to specify all necessary dependencies, especially peer dependencies for packages that need to share protobuf types.

this PR

  • adds a peer dependency to every package that uses buf registry packages
  • adds prod dependencies to apps that use workspace-root dependencies in prod
  • adds dependencies such as react and tailwindcss to the root, in order to ensure consistency.
  • adds the tool syncpack to manage dependency versions across packages in the monorepo

syncpack configuration

  • syncpack seems to not understand versions announced by the buf registry, so for @buf namespace it is configured to snap every package to the versions specified in the root package.
  • package.jsons are now consistently sorted and linted
  • i did not configure semver groups
  • i removed the inconsistently applied "license" field from each package in favor of a top-level license configration that matches the core repository
  • i added format/check scripts to package.json

use of syncpack

in the future when upgrading buf registry packages, upgrades should be done from the root, and then propagated into the monorepo with syncpack fix-mismatches

other packages which contain any reference in the root, when upgraded in any app or package within the repo, will propagate the highest referenced semver with syncpack fix-mismatches

potential todos

@turbocrime turbocrime requested a review from grod220 April 19, 2024 02:29
@grod220
Copy link
Collaborator

grod220 commented Apr 19, 2024

the only solution to this is to specify all necessary dependencies

Yeah, we have mistakenly made a convenience tradeoff by putting everything in the root.

adds the tool syncpack to manage dependency versions across packages in the monorepo

Maybe I'm missing the role of syncpack here. Is it meant to replace changeset? Or does it just go through the root package.json and ensure all of the packages package.json are consistent with the root's versions? For instance, by adding react to the root, it automatically enforces children to be in sync?

adds a peer dependency to every package that uses buf registry packages

Think we should make these dev dependencies so consumers are not required to independently install them.

when upgrading buf registry packages, upgrades should be done from the root

Can you add documentation on how the new developer workflow should be with syncpack?

Can you add a CI/CD check to ensure there aren't any mismatches?

package.json Outdated
Comment on lines 25 to 28
"pnpm": {
"overrides": {
"typescript": "^5.4.3"
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, sorry, that isn't strictly necessary. it forces api-extractor to depend on the same version of typescript as our build, which had the benefit of making vite-plugin-dts shut up until a few days ago

@grod220
Copy link
Collaborator

grod220 commented Apr 19, 2024

Noticed that dev dependencies and peer dependencies seem to be redundant in a number of packages. We should just move them into the dev or normal deps list and not require peers.

@turbocrime
Copy link
Contributor Author

turbocrime commented Apr 20, 2024

the only solution to this is to specify all necessary dependencies

Yeah, we have mistakenly made a convenience tradeoff by putting everything in the root.

It wasn't feasible to do it another way - if you remember, we started hoisting in order to solve type errors from peer conflicts. Now that we're publishing, we have to bring them back down.

Maybe I'm missing the role of syncpack here. Is it meant to replace changeset? Or does it just go through the root package.json and ensure all of the packages package.json are consistent with the root's versions? For instance, by adding react to the root, it automatically enforces children to be in sync?

The syncpack readme is quite informative: https://github.com/JamieMason/syncpack

fix-mismatches

Ensure that multiple packages requiring the same dependency define the same version, so that every package requires eg. react@16.4.2, instead of a combination of react@16.4.2, react@0.15.9, and react@16.0.0.

format

Organise package.json files according to a conventional format, where fields appear in a predictable order and nested fields are ordered alphabetically. Shorthand properties are used where available, such as the "repository" and "bugs" fields.

lint

Lint all versions and ranges and exit with 0 or 1 based on whether all files match your Syncpack configuration file.

There doesn't seem to be another tool for handling these tasks.

adds a peer dependency to every package that uses buf registry packages

Think we should make these dev dependencies so consumers are not required to independently install them.

peer dependencies are automatically installed since npm 7 released in 2020. any user interacting with our packages will certainly import from our message packages, and so must be aware of them and will likely choose them as deps. of course we should document such necessary things, but general consumption docs should not be part of this pr.

if we don't use peer dependencies, it could result in users installing incompatible packages, which may happily use incompatible message types as direct dependencies until the user tries to use the packages together. this leads to confusing type errors.

additionally, any consumer dependency on the buf registry will likewise be resolved separately from our packages' dependencies, and lead to similar type conflicts.

peer dependencies are the mechanism to resolve this, as they instruct the package manager to consider the needs of packages sharing the dependency as it selects dependency versions. so peer dependencies should be used if we expect third parties to consume these packages.

pnpm has a great document on how peer dependencies work https://pnpm.io/how-peers-are-resolved

when upgrading buf registry packages, upgrades should be done from the root

Can you add documentation on how the new developer workflow should be with syncpack?

ok, i will write that and update the pr. it's essentially impossible to do incorrectly - syncpack just suggests a fix if a mismatch is detected.

Can you add a CI/CD check to ensure there aren't any mismatches?

syncpack is the tool used to ensure there aren't any mismatches. it runs in ci already in this pr as part of the format-check script. i can add some additional config.

Noticed that dev dependencies and peer dependencies seem to be redundant in a number of packages. We should just move them into the dev or normal deps list and not require peers.

this is duplication is necessary.

@turbocrime
Copy link
Contributor Author

turbocrime commented Apr 20, 2024

i removed the inconsistently applied "license" field from each package in favor of a top-level license configration that matches the core repository

cc @hdevalence @zbuc or someone else from core team on this point - is this appropriate

@hdevalence
Copy link
Member

Yes, thanks for checking — MIT+Apache2 is the desired license.

@turbocrime turbocrime force-pushed the turbocrime/syncpack branch 2 times, most recently from 2689e5c to 38e5590 Compare April 20, 2024 18:53
Copy link
Collaborator

@grod220 grod220 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Helpful docs + explanation 👍

Let's carve out 10 mins at the start of Thursday's team sync for you to demo, explain any new workflows, and take some questions.

@turbocrime turbocrime force-pushed the turbocrime/syncpack branch from 38e5590 to d2fdb1d Compare April 22, 2024 16:04
@turbocrime turbocrime self-assigned this Apr 22, 2024
@turbocrime turbocrime merged commit 243f3f6 into main Apr 22, 2024
6 checks passed
@turbocrime turbocrime deleted the turbocrime/syncpack branch April 22, 2024 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants