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

Update pkg.ts #11

Closed
wants to merge 1 commit into from
Closed

Update pkg.ts #11

wants to merge 1 commit into from

Conversation

o-az
Copy link

@o-az o-az commented Aug 26, 2024

https://pnpm.io/cli/dlx

Describe the pull request

make pnpm exec work

Why

the following doesn't work

pnpm graphqurl https://spacex-production.up.railway.app --introspect

whereas this works

pnpm dlx graphqurl https://spacex-production.up.railway.app --introspect

How

How were these changes implemented?

Screenshots

If applicable, add screenshots to help explain what is being modified.

https://pnpm.io/cli/dlx

the following doesn't work
```sh
pnpm graphqurl https://spacex-production.up.railway.app --introspect
```
whereas this works
```sh
pnpm dlx graphqurl https://spacex-production.up.railway.app --introspect
```
Copy link

vercel bot commented Aug 26, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
starlight-package-managers ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 26, 2024 1:05pm

Copy link
Owner

@HiDeoo HiDeoo left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution 🙌

I think the current behavior is correct as it's matching the pnpm exec one which says:

The exec part is actually optional when the command is not in conflict with a builtin pnpm command […]

Altho, I think we could add a totally new command type, e.g. dlx? What do you think?

@o-az
Copy link
Author

o-az commented Sep 2, 2024

Thanks for your contribution 🙌

I think the current behavior is correct as it's matching the pnpm exec one which says:

The exec part is actually optional when the command is not in conflict with a builtin pnpm command […]

Altho, I think we could add a totally new command type, e.g. dlx? What do you think?

I think that makes sense! Since pnpm has

pnpm, pnpm run,
pnpm exec,
pnpm dlx

@o-az
Copy link
Author

o-az commented Sep 2, 2024

although just to be clear @HiDeoo, pnpm exec is not the same as npx since it doesn't attempt to install the package.

@HiDeoo
Copy link
Owner

HiDeoo commented Sep 2, 2024

although just to be clear @HiDeoo, pnpm exec is not the same as npx since it doesn't attempt to install the package.

This is the case altho based on "npx vs npm exec", I've seen people mostly interchanging them but prefering the npx argument parsing logic.

@o-az
Copy link
Author

o-az commented Sep 9, 2024

currently with this setup:

<PackageManagers
  pkg="jsr"
  type="exec"
  frame="none"
  args="add @union/client"
  pkgManagers={['pnpm']}
/>

it renders:

pnpm jsr add @union/client

which when ran results in:

 ERR_PNPM_RECURSIVE_EXEC_FIRST_FAIL  Command "jsr" not found

whereas pnpm dlx jsr add @union/client works as expected.

You suggest I update the pr and add a new command for pnpm dlx?

@HiDeoo
Copy link
Owner

HiDeoo commented Sep 10, 2024

I think this is what we agreed on and still makes sense. I would be up to the user to use the appropriate syntax that will work based on the situation, we cannot predict what is the desired behavior in this case if I'm not missing something.

jits added a commit to jits/starlight-package-managers that referenced this pull request Oct 31, 2024
From the discussion in HiDeoo#11

I've mirrored the implementation of the `exec` command.

Note: for pnpm, whilst there is an alias — `pnpx` — I've chosen to use `pnpm dlx`, as the alias was only introduced in pnpm v9 (Apr 2024).
jits added a commit to jits/starlight-package-managers that referenced this pull request Oct 31, 2024
From the discussion in HiDeoo#11

I've mirrored the implementation of the `exec` command.

Note: for pnpm, whilst there is an alias — `pnpx` — I've chosen to use `pnpm dlx`, as the alias was only introduced in pnpm v9 (Apr 2024).
@HiDeoo
Copy link
Owner

HiDeoo commented Nov 1, 2024

Thanks to #15, the version 0.8.0 now includes the dlx command type 🎉

@HiDeoo HiDeoo closed this Nov 1, 2024
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