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

fix: do not move packages that are not shared to catalog #6

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mcous
Copy link

@mcous mcous commented Nov 8, 2024

Hello, thanks for this codemod! I tried it out in a monorepo, and noticed this aspect of the docs was not true:

If there are more than 2 packages with same dependency - it will move dependency to catalog.

Instead, the codemod moved all dependencies to the catalog, which wasn't what I wanted. In case the difference was unintentional, this PR updates the codemod to behave according to the docs. In addition to tracking the version count, it also tracks the dependent count, so we only select dependencies that have a single version used across multiple dependencies

@deanc
Copy link

deanc commented Feb 4, 2025

Same observation here. This codemod did the exact opposite of what it said it would do. It "catalogged" all the packages that were used once, and none of those used 2+ times.

@zkochan
Copy link
Member

zkochan commented Feb 4, 2025

I think moving everything to catalogs is also a valuable feature. Do codemods support flags or options to support both?

@mcous
Copy link
Author

mcous commented Feb 5, 2025

I think moving everything to catalogs is also a valuable feature. Do codemods support flags or options to support both?

@zkochan I've not worked with codemods before this PR, but a quick glance at their docs seem to indicate that yes, arguments are supported. Would you like me to update this PR with argument(s) to support both the current behavior of moving all packages and the docs-stated behavior of moving duplicated packages? If so, do you have any preferences on the argument names and/or defaults?

Some sort of "threshold" argument would be easy to tack on to this PR without much fuss - i.e. "if [N] or more packages have this dependency, move it to the catalog"

@zkochan
Copy link
Member

zkochan commented Feb 5, 2025

I guess it depends on what will be the default. We could have something like --duplicates-only or --all-deps.

@mcous
Copy link
Author

mcous commented Feb 6, 2025

"Only duplicates" was the behavior I was seeking out as a user to try catalogs for the first time. I think it's a sensible default while the catalogs feature is still relatively new and isn't fully integrated into stuff like pnpm update.

That being said, as long as both are possible I don't much mind either way. I defer to your judgement if you feel "all dependencies" is a better default

@zkochan
Copy link
Member

zkochan commented Feb 6, 2025

I wasn't the one who created the codemod. If the README says that only duplicates are moved to catalogs, then I am OK with that being the default.

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