Skip to content

chore: switch from preferred-pm to package-manager-detector#1446

Merged
Andarist merged 3 commits intochangesets:mainfrom
benmccann:package-manager-detector
Aug 28, 2024
Merged

chore: switch from preferred-pm to package-manager-detector#1446
Andarist merged 3 commits intochangesets:mainfrom
benmccann:package-manager-detector

Conversation

@benmccann
Copy link
Copy Markdown
Contributor

@benmccann benmccann commented Aug 28, 2024

https://npmgraph.js.org/?q=preferred-pm@3.0.0 - 34 dependencies
https://npmgraph.js.org/?q=package-manager-detector - 0 dependencies

A dozen dependencies are in the changesets dependency tree elsewhere, so the real savings for now will be 21 dependencies (18% of the overall changesets total). Half of the dependencies that appear elsewhere in the dependency tree have been removed from the latest version of the @manypkg packages and so this change together with upgrading those packages will result in an even greater savings in the future.

The yarn lockfile is only a little smaller because various devDependencies are pulling in many the removed dependencies. E.g. jest ends up pulling in pkg-dir. However, I see a greater savings in my projects. E.g. I use vitest rather than jest, so it is a really large savings for many users even if they're not visible in yarn lockfile here.

package-manager-detector was recently refactored out from the widely used @antfu/ni and @antfu/install-pkg as a new package by the authors of those packages upon my request. While the package itself is fairly new, the logic in it has been used for a long time

Before this, I had taken another approach of contributing to reducing the size of preferred-pm. However, when I tried to upgrade changesets to use the latest, I realized that won't work because the latest versions of preferred-pm require Node 18 and changesets supports older versions of Node.

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Aug 28, 2024

🦋 Changeset detected

Latest commit: 81939f2

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@changesets/cli Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@codesandbox-ci
Copy link
Copy Markdown

codesandbox-ci Bot commented Aug 28, 2024

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

@codecov
Copy link
Copy Markdown

codecov Bot commented Aug 28, 2024

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 81.02%. Comparing base (4efc038) to head (81939f2).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
packages/cli/src/commands/publish/npm-utils.ts 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1446   +/-   ##
=======================================
  Coverage   81.02%   81.02%           
=======================================
  Files          54       54           
  Lines        2213     2213           
  Branches      658      654    -4     
=======================================
  Hits         1793     1793           
  Misses        416      416           
  Partials        4        4           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

import { AccessType, PackageJSON } from "@changesets/types";
import pLimit from "p-limit";
import preferredPM from "preferred-pm";
import { detect } from "package-manager-detector/detect";
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this path~ won't work in node versions that don't support package.json#exports. Our support of older node versions isn't precisely defined 😢 at this point it would probably be best to just publish Changesets v4 with required node 18 or smth but it would require me to do some prep for that and it's not something that is on the top of my priority list. If we could prove that Changesets don't work on pre-12 version either way then this would be OK as well but it's something that might be tricky to prove.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

but honestly, we probably should just do it - i mean, add appropriate engines everywhere and release a new major. Perhaps I don't have to batch this release with any other breaking changes. This would simplify things. I'd only have to give some thought to the impact on changesets/action and changesets/bot

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This method is also exported from the default export. I've updated to get it from there, which avoid this issue and lets us kick the can down the road a bit longer

Comment thread .changeset/slow-avocados-promise.md Outdated
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.

2 participants