feat: add strategies option for detect API #42
Conversation
|
I would love to hear more about when this would be helpful (like you are debugging but still want to call some installation programmatically?). Does this also means there might be multiple results in a same project? |
Yeah that's the main scenario I can think of. I wanted to replace this in Astro where it has an Maybe it's fine if
Yes. It could be thought as "the package manager used for installation" and "the package manager preferred for other commands" for me. |
|
Or maybe if we introduce a ENV flag to override that detect agent, which would cover your debugging case and being a bit more explicit? WDYT? |
|
The intention is to only use a different package manager when doing installation, so using an env override that also changes the package manager detected in other cases feels wrong to me I think. Also it's easy to forget or hard to know about the flag that in which at that point I think this feature isn't worth it. If we're on the fence of this feature (personally I am a little too), maybe we could try to simply open a PR on astro side as see what they think? I figured the feature wasn't hard to implement so I open a PR here first. |
There was a problem hiding this comment.
Do we need two different methods? If we just added this functionality to detect I think it would be simpler for users. It's hard to tell the difference apart between these methods. And it doesn't seem like you'd have someone install node_modules with a package manager, delete that package manager's lockfile, and then look for a different lockfile?
I'm guessing you added it as a second method to avoid a breaking change, but if we're talking about making a breaking change anyway in #44 then maybe it'd simplify the API here to just make a change to the existing method
src/detect.ts
Outdated
| * @param options {DetectOptions} The options to use when detecting the package manager. | ||
| * @returns {DetectResult | null} The detected package manager or `null` if not found. | ||
| */ | ||
| export function detectInstallSync(options: DetectOptions = {}): DetectResult | null { |
There was a problem hiding this comment.
maybe drop the sync version per #44
src/detect.ts
Outdated
| @@ -63,6 +63,64 @@ export function detectSync(options: DetectOptions = {}): DetectResult | null { | |||
| return null | |||
| } | |||
|
|
|||
| /** | |||
| * Detects the package manager used for installation in the project. | |||
There was a problem hiding this comment.
I think it's quite difficult to tell from the JSDocs what the difference is between detect and detectInstall
|
I personally don't find it confusing.
A more probable scenario would be like: You downloaded a repro that uses It's not clear to me what's the direction we want to go in this PR to make a change. |
|
Maybe having a single |
|
I quite like that. Alternatively would making it usecase-oriented be useful too? |
Right, that was my point. I don't know when you wouldn't want to employ all strategies. Is there ever a case where it would hurt to use them all and you'd want it to return bun? |
|
Perhaps after installing with pnpm you'd want to print out "bun run some-command" or something? If you use a package manager to install it doesn't mean you'd want to use it for every other command. We could definitely put a stand otherwise if that's what we want the behaviour to be, I don't have a hard preference but maybe some folks prefer different results in different occasions. In the context of me wanting to replace this in astro add, I think it's also fine if detect always account for node_modules. An option to change the behaviour would be a bonus. |
I think in a way we are not an enduser-facing library. I think transparent might be better to have over easy-to-config. I also found "install" still a bit confusing to me. With more technically named |
|
I've updated the code to implement One thing I'm unsure of is that |
|
I see, for yarn it's indeed not very obvious for name it |
|
I like Maybe rename |
…ckageManager-field
|
I've renamed to |
|
Hey @bluwy , when you have time, can you add some entry to the readme file 🙏 ? |
Description
Currently the
detectAPIs uses the lock files for detection, however the lock files don't necessarily reflect the package manager used for installation. For example, when debugging a project that usespnpm-lock.yaml, and then making an install with npm that generatespackage-lock.json, the correct package manager used for installing things is npm.This behaviour is similar to https://www.npmjs.com/package/which-pm, which I wanted to replace with this but the lock file behaviour would yield different results.
I'm not sure if this is out-of-scope of the project. If so, I don't mind if this PR is closed.
Linked Issues
n/a
Additional context
Alternatively, perhaps the installation detection can be part of the main
detectAPI so we don't need two APIs, but this would be a breaking change.