-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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(core): fix package manager determination logic #28202
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
☁️ Nx Cloud ReportCI is running/has finished running commands for commit e57b851. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this CI Pipeline Execution ✅ Successfully ran 4 targets
Sent with 💌 from NxCloud. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is working as intended based on the e2e results.
Based on my own experimentation I believe what we want is to replace the existing logic (and your change) with this:
if (process.env.npm_config_user_agent) {
for (const pm of packageManagerList) {
if (process.env.npm_config_user_agent.startsWith(pm)) {
return pm;
}
}
}
if (process.env.npm_execpath) {
for (const pm of packageManagerList) {
if (process.env.npm_execpath.includes(pm)) {
return pm;
}
}
}
return 'npm';
(Obviously make sure the formatting is fixed before pushing)
Results on my machine:
@JamesHenry indeed - I didn't have intention to say it's a final code. So far I've been trying still to understand what is the functional meaning of this function. Like what is more important - to understand what is running the code or which npm was used during installation because is not the same. Please note that if the code is run via node or deno directly - npm_config_user_agent is empty. |
@JamesHenry is there any way to automate formatting before pushing? like husky + format:write may be? |
OK I see now - seems like we are supposed to use pm used to trigger the call. In case if there is no package manager ( let's say we call globally installed create-nx-workspace or via node programmatically ) - then we just use npm as fallback. @JamesHenry I intentionally use / in the startsWith to make sure it's yarn and node something like yarn_alternative. In the includes script I break path into segments and we search for a segment = npm | yarn | bun. And segments like ubuntu will not be taken into account
|
@ThePlenkov thanks looks reasonable |
I'm struggling to understand why test is failing. It is failing even locally when I run it on the upstream branch ( without my actual changes ). Can it be because of remote cache? |
@ThePlenkov I have brought things up to date with the latest, are you able to reproduce locally? |
I did, but couldn't reproduce failing tests(( |
@ThePlenkov I think I have traced the issue, however I cannot push directly to the PR because you opened it from your Please can you try removing this hardcoded npm reference in the e2e utils, I am not sure why it was done but I believe it could be the cause of the error because based on the logs, the test workspace is getting created with pnpm but then is attempting an |
@JamesHenry I'll create another PR better then |
This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request. |
Current Behavior
Currently create-nx-workspace determines package manager based on the invoker's path and it might be totally misleading if some names are used in the path such as /ubuntu/my_project which is identified as bun command by mistake
Expected Behavior
It should correctly identify package manager
Related Issue(s)
Fixes #28201