-
-
Notifications
You must be signed in to change notification settings - Fork 6k
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 pkg.pr.new #17314
feat: add pkg.pr.new #17314
Conversation
|
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. |
Thanks @AmirSa12! I'll discuss with the rest of the team about this PR once it the issue is solved 👍🏼 |
@patak-dev, the app is not installed on this repo. |
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? |
The github app is used mainly for authentication and avoiding spam publishes.
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. |
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. |
Yes, nothing else is required!
You got it perfectly right, it's not a js registry. Let's say it's an npm tarball/artifact host 😅 |
The app is now installed @AmirSa12. Great to see you fixing that npm bug upstream! |
vite (
|
@AmirSa12 @Aslemammad about the generated message, is this generated only once per PR and then updated? Some comments about it:
![]() 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 ![]() |
Thank you for the helpful suggestions, we'll address as many as possible and keep you updated here. |
waiting for approval to see if everything is working as expected. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving to test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing
Thank you! Everything seems to be working fine. |
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 |
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. |
.github/workflows/cr.yml
Outdated
- name: Build | ||
run: pnpm build | ||
|
||
- run: pnpx pkg-pr-new publish --compact --pnpm ./packages/vite |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pnpm dlx
is now recommended over pnpx
. Could we use it instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@patak-dev we can do that, we were working on issues so we used latest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dominikg, Thanks! Yes lets run build only in packages/vite since we only publish that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one thing you can add to enhance security even in the event the token is stolen is
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?
Description
This PR adds pkg.pr.new to the repo