feat: add pkg.pr.new

[AmirSa12]

Description

This PR adds pkg.pr.new to the repo

[stackblitz]

Run & review this pull request in StackBlitz Codeflow.

[AmirSa12]

we encountered an issue with npm and we are trying to solve it. I marked this as a draft PR until then.

EDIT: created a PR related to an upstream issue in npm npm/cli#7579. Currently, we have decided to switch to pnpm for this case.

[patak-dev]

Thanks @AmirSa12! I'll discuss with the rest of the team about this PR once it the issue is solved 👍🏼

[AmirSa12]

@patak-dev, the app is not installed on this repo.

[bluwy]

Not opposing this addition as I'd like to support preview releases too, but is there a reason for a GitHub app for this? Couldn't it fit entirely in CI with a secret token, like publishing to npm?

[Aslemammad]

is there a reason for a GitHub app for this

The github app is used mainly for authentication and avoiding spam publishes.

Couldn't it fit entirely in CI with a secret token, like publishing to npm?

One of the characteristics of pkg.pr.new is that it does not publish to npm so it does not make the npm history ugly and instead publishes the package to the pkg.pr.new domain. Which means way more control and flexibility for later removals for instance.

[bluwy]

IIUC the GitHub app would be the way to authenticate and start using pkg.pr.new, and no other sign in is required? I can get by a GitHub app in that case 👍

I get that it's not relying on npm, but I thought it was sort of a js registry so it could have a similar auth flow as npm. I guess it may not be possible to publish locally in the future, but perhaps it's intentionally out of scope.

[Aslemammad]

no other sign in is required

Yes, nothing else is required!

I guess it may not be possible to publish locally in the future

You got it perfectly right, it's not a js registry. Let's say it's an npm tarball/artifact host 😅

[patak-dev]

The app is now installed @AmirSa12. Great to see you fixing that npm bug upstream!

[pkg-pr-new]

vite (77b7137)

npm i https://pkg.pr.new/vite@17314

[patak-dev]

@AmirSa12 @Aslemammad about the generated message, is this generated only once per PR and then updated? Some comments about it:

• We already have the StackBlitz app, so it would be good to check if there is already a Review PR button and avoid adding another one.
• If Open in Codeflow is exactly the same as the Review PR one, we should align their sizes and use the same wording in general
• We probably don't need the types and dep-types lines here, no? Probably a bug on pkg.pr.new side
• Maybe a good idea to detect what package manager the project is using and use that instead of always npm in the code snippet.
• It is taking too much space in the discussion in general, even if it is a single one and the above are removed. I would expect something closer to

Titles could be smaller too, image just for reference.

About the PR vs commit hash, I wonder if a single URL with the PR would be good enough here. This is what normally you'd want to use in a PR. So in that case, it would be even better. If that is the case, including the title with the PR number is not needed as we are already in that PR. So it could be just

[Aslemammad]

Thank you for the helpful suggestions, we'll address as many as possible and keep you updated here.

[AmirSa12]

waiting for approval to see if everything is working as expected.

[patak-dev]

Approving to test

[patak-dev]

Test

[patak-dev]

Testing

[AmirSa12]

Thank you! Everything seems to be working fine.

[patak-dev]

Should we add the last modified date too? Right now it requires the user to click on the Edited dropdown to see that. Maybe this is ok though. I'm good with moving forward without it.

The hash link is currently pointing to https://github.com/vitejs/vite/runs/26016615176
That page doesn't link to the commit inside. Is this intentional?
I was expecting a direct link to the commit in the bot message.

[patak-dev]

Ok, I discussed with @Aslemammad and having the date is challenging due to time zones (something to look for later). And the current link has the commit name and link when you navigate to it, so I'm good with that too.

[bluwy]

pnpm dlx is now recommended over pnpx. Could we use it instead?

[patak-dev]

Actually, I forgot @dominikg's feedback. @AmirSa12 we should move to pkg-pr-new being a dev dependency, no?

[dominikg]

that would be great, yes. Another option would be to use an explicit version here pnpm dlx [email protected]

Also if this only publishes vite, then maybe it's cheaper to run build only in packages/vite?

[AmirSa12]

@patak-dev we can do that, we were working on issues so we used latest.

[AmirSa12]

@dominikg, Thanks! Yes lets run build only in packages/vite since we only publish that

[patak-dev]

I think we should go with pnpm dlx [email protected] instead of the dev dependency, so we don't get this locked at the patch level in our package lock. This should give us the latest 0.0.x version IIUC, that would be useful now that the project is releasing bug fixes often.

[dominikg]

one thing you can add to enhance security even in the event the token is stolen is

https://github.com/sveltejs/vite-plugin-svelte/blob/45a43a4084499559a2e7a08b2403729e835fa06a/.github/workflows/ci.yml#L20

https://docs.github.com/en/actions/using-jobs/assigning-permissions-to-jobs

with that, the script not needing the token by default and the fact that we know and trust the org that controls the pkg-pr-new package it could be fine to keep this unpinned if it helps development.

But i kind of wonder if it is a good idea to automate it already if it is still undergoing frequent changes. Wouldn't a comment trigger like with ecosystem-ci be a slower start where you can learn about what works?