vite icon indicating copy to clipboard operation
vite copied to clipboard

feat: add pkg.pr.new

Open AmirSa12 opened this issue 1 year ago • 3 comments

Description

This PR adds pkg.pr.new to the repo

AmirSa12 avatar May 25 '24 12:05 AmirSa12

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

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.

AmirSa12 avatar May 25 '24 12:05 AmirSa12

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

patak-dev avatar May 25 '24 12:05 patak-dev

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

AmirSa12 avatar Jun 03 '24 15:06 AmirSa12

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?

bluwy avatar Jun 04 '24 02:06 bluwy

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.

Aslemammad avatar Jun 04 '24 02:06 Aslemammad

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.

bluwy avatar Jun 04 '24 02:06 bluwy

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 😅

Aslemammad avatar Jun 04 '24 02:06 Aslemammad

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

patak-dev avatar Jun 04 '24 05:06 patak-dev

vite (77b7137)

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

pkg-pr-new[bot] avatar Jun 04 '24 05:06 pkg-pr-new[bot]

@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 image

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

image

patak-dev avatar Jun 04 '24 06:06 patak-dev

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

Aslemammad avatar Jun 04 '24 07:06 Aslemammad

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

AmirSa12 avatar Jun 05 '24 16:06 AmirSa12

Thank you! Everything seems to be working fine.

AmirSa12 avatar Jun 10 '24 10:06 AmirSa12

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. image

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 avatar Jun 10 '24 10:06 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.

patak-dev avatar Jun 10 '24 10:06 patak-dev