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

feat: add @yarnpkg/tools package with detectPackageManager function #3408

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

paul-soporan
Copy link
Member

@paul-soporan paul-soporan commented Sep 8, 2021

What's the problem this PR addresses?

There's currently no standardized way for third-party tools to detect which package manager should be used inside the project, which leads to each tool implementing a different set of heuristics that don't handle all of the edge cases.

How did you fix it?

I created a new @yarnpkg/tools package containing a detectPackageManager function to be used by any tools needing to pick a package manager to use either inside a project or when init-ing a new project.

TODO:

  • Fallback to corepack elect at the end (Implements "corepack elect" nodejs/corepack#26)
  • Show warnings when there are lockfiles in the home folder that could mess up the pm detection
  • Tests, many tests
  • Replace detectPackageManager from scriptUtils (in @yarnpkg/core) with detectPackageManagerFromProject
  • Possibly add an async implementation too

Checklist

  • I have set the packages that need to be released for my changes to be effective.
  • I will check that all automated PR checks pass before the PR gets reviewed.

@andreialecu
Copy link
Contributor

andreialecu commented Sep 10, 2021

Thanks for this. As for more context on why this is going to be super useful, here are two frameworks that try to detect the package manager but don't handle all the edge cases (particularly they can't detect when used inside workspaces):

Angular: https://github.com/angular/angular-cli/blob/c1512e42742c17ace82e783e8e9c919ae925d269/packages/angular/cli/utilities/package-manager.ts#L34-L58
Nest: https://github.com/nestjs/nest-cli/blob/2271c9e0ba6be1c7a202ef1b67d81407a0ac26d5/lib/package-managers/package-manager.factory.ts#L22-L41


An opinion on the name @yarnpkg/tools. While it does look cool, I think it implies that it is something internal to @yarnpkg/* instead of being meant for public consumption.

I'm not sure what a good name would be, going to list some random names for possible inspiration:

@yarnpkg/detect-package-manager (might imply it only detects yarn)
@yarnpkg/which-package-manager
@yarnpkg/package-manager-tools

@yarnpkg/whichpm
@yarnpkg/detectpm
@yarnpkg/pmtools

@yarnpkg/package-manager-manager 😆
@yarnpkg/pmm

@yarnpkg/devtools
@yarnpkg/pm-devtools

I think a good name would be self describing, so just by glancing at it inside a package.json you could get an idea about what it's supposed to be for.

@merceyz
Copy link
Member

merceyz commented Sep 10, 2021

The name really depends on what we want this package to do, @paul-soporan do you have any plans of adding install, add, remove, update functions as well?

To add to the examples @andreialecu provided, cordova could also benefit from this. apache/cordova-cli#292 (comment)

Possibly add an async implementation too

You can use https://github.com/loganfsmyth/gensync to write it once while supporting both sync, async, and callback based functions.
Example here https://github.com/babel/babel/blob/b2d9156cc62d37f4c522c9505a00f50b99a1eb74/packages/babel-core/src/transform-file.ts#L26-L40

return {
name: PackageManager.YARN,
reason: `Found "${field}" in npm_config_user_agent`,
isClassic: value.startsWith(`0`) || value.startsWith(`1`),
Copy link
Contributor

Choose a reason for hiding this comment

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

This will be a problem with Yarn 10 😆

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