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 getCurrentPackageManager #9505

Merged
merged 4 commits into from Jan 26, 2024

Conversation

bestlyg
Copy link
Contributor

@bestlyg bestlyg commented Jan 25, 2024

↪️ Pull Request

💻 Examples

my repo is monorepo by pnpm-workspace protocal and is installed with pnpm.
It will create pnpm-lock yaml in root directory.
When I run parcel build with types in subpackage, it will auto install @parcel/transformer-typescript-types with yarn.

I check the source code and find the result is that the library of package manager will detect current package manager by lockfile.
If the current work directory has no lockfile, it will run in ['yarn', 'pnpm', 'npm'] order.
It is not my expect. I want to it works for pnpm.

So I want to add a function named getCurrentPackageManager, it will detect the current package manager with process.env.npm_config_user_agent.

🚨 Test instructions

✔️ PR Todo

  • Added/updated unit tests for this change
  • Filled out test instructions (In case there aren't any unit tests)
  • Included links to related issues/PRs

@mischnic
Copy link
Member

projectRoot in that function either contains a lockfile or a .git (and only if none of these files exist in any parent directory, is is the current working directory). So in your case, that should already be the correct folder and find your lockfile.

@bestlyg
Copy link
Contributor Author

bestlyg commented Jan 26, 2024

projectRoot in that function either contains a lockfile or a .git (and only if none of these files exist in any parent directory, is is the current working directory). So in your case, that should already be the correct folder and find your lockfile.

Thanks for your reply.
I make a demo to explain the issue.
reprocude steps:

  1. pnpm i in root directory.
  2. cd packages/core
  3. pnpm build

you can find that the parcel will install dependencies with yarn.
image

The projectRoot is correct owing to it contains.git.
Special case: I setted the lockfile=false in .npmrc, because I do not need lockfile in my actual project.
The determinePackageInstaller function only reolve lockfile.
so I want to add the function, it can help parcel find current package manager that is called.

@mischnic
Copy link
Member

👍

  • You need to run yarn flow and fix all type errors. The function declaration should be this
export default function getCurrentPackageManager(
  userAgent: ?string = process.env.npm_config_user_agent,
): ?{|name: string, version: string|} {
  if (userAgent == null) {
    return;
  }
  • Instead of log:agent, just hardcode some current pnpm/npm/yarn user agent strings and check them. The CI probably doesn't have pnpm installed for example

@bestlyg
Copy link
Contributor Author

bestlyg commented Jan 26, 2024

👍

  • You need to run yarn flow and fix all type errors. The function declaration should be this
export default function getCurrentPackageManager(
  userAgent: ?string = process.env.npm_config_user_agent,
): ?{|name: string, version: string|} {
  if (userAgent == null) {
    return;
  }
  • Instead of log:agent, just hardcode some current pnpm/npm/yarn user agent strings and check them. The CI probably doesn't have pnpm installed for example

Thanks for your reply.
I have updated the type of getCurrentPackageManager and hardcode the test file.
I have run yarn flow and yarn test and reported no error.

@mischnic
Copy link
Member

There are some eslint errors in packages/core/package-manager/test/getCurrentPackageManager.test.js

@bestlyg
Copy link
Contributor Author

bestlyg commented Jan 26, 2024

There are some eslint errors in packages/core/package-manager/test/getCurrentPackageManager.test.js

Thanks for remind.
I have fixed it.

Copy link
Member

@mischnic mischnic left a comment

Choose a reason for hiding this comment

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

Thanks!

@mischnic mischnic merged commit fdf495b into parcel-bundler:v2 Jan 26, 2024
13 of 16 checks passed
lettertwo added a commit that referenced this pull request Jan 30, 2024
* upstream/v2: (22 commits)
  Add source map support to the inline-require optimizer (#9511)
  [Web Extension] Add content script world property to manifest schema validation (#9510)
  feat: add getCurrentPackageManager (#9505)
  Default Bundler Contributor Notes (#9488)
  rename parentAsset to root for msb config and remove unstable (#9486)
  Macro errors -> v2 (#9501)
  Statically evaluate constants referenced by macros (#9487)
  Multiple css bundles in Entry bundle groups issue (#9023)
  Fix macro issues (#9485)
  Bump follow-redirects from 1.14.7 to 1.15.4 (#9475)
  Revert more CI changes to centos job (#9472)
  Use lightningcss to implement CSS packager (#8492)
  Fixup CI again (#9471)
  Clippy and use napi's Either3 (#9047)
  Upgrade to eslint 8 (#8580)
  Add support for JS macros (#9299)
  Fixup REPL CI (#9467)
  Drop per-pipeline transformation cache (#9459)
  Upgrade some CI actions (#9466)
  REPL (#9365)
  ...
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.

None yet

2 participants