chore: upgrade package-manager-detector to add Deno support

[benmccann]

No description provided.

[changeset-bot]

🦋 Changeset detected

Latest commit: b736d8b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
sv	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[pkg-pr-new]

Open in Stackblitz

npm i https://pkg.pr.new/sveltejs/cli/sv@313

npm i https://pkg.pr.new/sveltejs/cli/svelte-migrate@313

commit: b736d8b

[AdrianGonz97]

It looks like the execute-local command for Deno (which maps to deno run {0}) isn't a valid way to execute local binaries, so it breaks during formatting (and will also break for sv check as we also depend on it there)

[benmccann]

I filed a feature request denoland/deno#26918 after discussing it in https://github.com/antfu-collective/package-manager-detector/pull/25/files#r1847062219

[benmccann]

@AdrianGonz97 I've updated this with a new version that should hopefully work. Can you test again?

[AdrianGonz97]

Thanks! The previous issue is now resolved, but now we have another one that could be worked around.

Running the storybook add-on is failing as it's attempting to execute it with:

deno run storybook@latest init --skip-install --no-dev

which isn't correct when trying to execute remote scripts.

It should instead have the npm: protocol preprended to the name of the package:

deno run npm:storybook@latest init --skip-install --no-dev

This could be fixed this on our end, but it doesn't seem to be the intended purpose of the execute command for package-manager-detector.

This also raises the question of how should we deal with deno's security features? Should we just add a -A flag to permit all features (granting read/write/env/run access)? Or should we just let deno's built-in prompts take the wheel and have the user deal with it? I'm thinking the latter should be better, but the former is a nicer experience.

[manuel3108]

This could be fixed this on our end, but it doesn't seem to be the intended purpose of the execute command for package-manager-detector.

Maybe I'm missing the point, but based on denoland/deno#26918 i would have expected something like this:

deno task --eval "storybook@latest init --skip-install --no-dev"

This also raises the question of how should we deal with deno's security features? Should we just add a -A flag to permit all features (granting read/write/env/run access)?

They chose deno for a reason, so I think it should be up to the end user to deal with stuff.

[AdrianGonz97]

Maybe I'm missing the point, but based on denoland/deno#26918 i would have expected something like this:

deno task --eval "storybook@latest init --skip-install --no-dev"

that's only to execute local binaries, not remote ones.

They chose deno for a reason, so I think it should be up to the end user to deal with stuff.

good point! we can just leave it as-is then

[benmccann]

@bartlomieju we've had to add some special logic here to support Deno:

if (workspace.packageManager === 'deno') commandArgs[0] = `npm:${commandArgs[0]}`;

I guess everyone else who uses package-manager-detector will need to do the same. I'm a bit fearful right now that other folks using package-manager-detector aren't going to know they have to do something special for Deno as they're probably only testing their setups with a single package manager, which might leave it broken for most folks trying to use Deno. If we could have package-manager-detector continue to abstract away and hide all the differences between package managers I think that would be nice.

Do you think either Deno or package-manager-detector might be able to add automatically add the npm: prefix when there's no prefix?

[bartlomieju]

@benmccann some team members at Deno are in favor of adding --npm flag, sadly the soonest we can ship it is in Deno v2.2 in early January. I will raise this topic tomorrow during our weekly meeting, but for now it's probably better to go with npm: prefix to unblock the progress here.

[bartlomieju]

@benmccann I discussed it with the team and we feel the better solution would be to actually update package-manager-detector to prepend npm: to the command person wants to run. Should I open a PR myself?

[benmccann]

Sure that sounds good. I can merge that PR if you can send it. Thanks!!

[AdrianGonz97]

The new update for package-manager-detector isn't working either. The run and execute commands were flipped. Opened a PR there: antfu-collective/package-manager-detector#36

[benmccann]

Thanks for all the help testing this @AdrianGonz97. Want to give it another go?

[Rich-Harris]

preview: https://svelte-dev-git-preview-cli-313-svelte.vercel.app/

this is an automated message

[AdrianGonz97]

awesome! everything looks to be working perfectly now