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

feat: add pkg.pr.new #896

Closed
wants to merge 10 commits into from
Closed

Conversation

AmirSa12
Copy link
Contributor

@AmirSa12 AmirSa12 commented May 15, 2024

This PR adds pkg.pr.new to the repo

Copy link

vercel bot commented May 15, 2024

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

Name Status Preview Comments Updated (UTC)
valtio ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 19, 2024 4:35pm

@AmirSa12
Copy link
Contributor Author

AmirSa12 commented May 15, 2024

it's failing now because the app must be installed.
@dai-shi

@dai-shi
Copy link
Member

dai-shi commented May 15, 2024

I'm fine to add pkg.pr.new, but please don't remove codesandbox ci yet.

@dai-shi
Copy link
Member

dai-shi commented May 15, 2024

Try publishing from a github workflow or install https://github.com/apps/pkg-pr-new Github app on this repo

But, it says "or".

@Aslemammad
Copy link
Member

But, it says "or".

Yea, it's a bit misleading, we'll change the error message! But anyway, it requires pkg.pr.new app.

Copy link

codesandbox-ci bot commented May 15, 2024

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Copy link

pkg-pr-new bot commented May 15, 2024

Last Commit Build: af17d18

valtio(af17d18):

npm i https://pkg.pr.new/pmndrs/valtio/valtio@af17d18    

Pull Request Build: #896

valtio(#896):

npm i https://pkg.pr.new/pmndrs/valtio/valtio@pr-896    

@Aslemammad Aslemammad requested a review from dai-shi May 15, 2024 19:08
@dai-shi
Copy link
Member

dai-shi commented May 16, 2024

$ npm i https://pkg.pr.new/pmndrs/valtio/valtio@e688081
$ ls node_modules/valtio 
LICENSE			package.json		tsconfig.json
dist/			readme.md		vitest.config.ts
docs/			rollup.config.js	website/
examples/		src/
logo.svg		tests/

Unfortunately, it isn't built correctly.

FYI:

$ npm i https://pkg.csb.dev/pmndrs/valtio/commit/e6880814/valtio
$ ls node_modules/valtio 
LICENSE
esm/
index.d.ts
index.js
package.json
react/
react.d.ts
react.js
readme.md
ts_version_4.5_and_above_is_required.d.ts
utils.d.ts
utils.js
vanilla/
vanilla.d.ts
vanilla.js

Copy link
Member

@dai-shi dai-shi left a comment

Choose a reason for hiding this comment

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

☝️

@Aslemammad
Copy link
Member

@dai-shi What files are missing? the / type files?

@dai-shi
Copy link
Member

dai-shi commented May 16, 2024

It's too many files. We publish only dist.

With CSB:

"packages": ["dist"],

@AmirSa12
Copy link
Contributor Author

now I guess it's publishing the dist folder

@dai-shi
Copy link
Member

dai-shi commented May 16, 2024

https://github.com/pmndrs/valtio/actions/runs/9114322700/job/25057962260?pr=896 looks good.

I receive this email, and am wondering where it's coming from:

Date: Thu, 16 May 2024 07:41:02 -0700
From: Daishi Kato <notifications@github.com>
To: AmirSa12/valtio <valtio@noreply.github.com>
Cc: Ci activity <ci_activity@noreply.github.com>
Subject: [AmirSa12/valtio] Run failed: Publish Any Commit - feat/add-pkg-pr-new (af17d18)

This is not the first time. It seems something is wrong.

@Aslemammad
Copy link
Member

Weird, could you send the email screenshot? maybe it's a bug from our side that submits emails as errors (I don't think so tbh, but worth a try)

@AmirSa12
Copy link
Contributor Author

I always receive this email from GitHub notifications when a check is failed not only in this repo but, in all of the repos I open a PR in, and I think before you verify the commit, the checks go red, that's why all of the participants receive this email ( including you and Mohammad ) .

@dai-shi
Copy link
Member

dai-shi commented May 17, 2024

I think before you verify the commit, the checks go red

Are you sure? I never received this email before in other repos.

all of the participants receive this email ( including you and Mohammad ) .

So, Mohammad should receive it too.

@Aslemammad
Copy link
Member

Weirdly enough, I didn't receive it, maybe because I'm not part of the PR?

@AmirSa12 AmirSa12 requested a review from dai-shi May 18, 2024 07:52
@AmirSa12
Copy link
Contributor Author

AmirSa12 commented May 18, 2024

I think Mohammad is not receiving it because of he is not part of the PR.
This has to do with my GH notification configurations.
in the attached image, you can see that I received this email in 3 different PRs when the checks failed
photo_2024-05-18_11-28-28

Copy link
Member

@dai-shi dai-shi left a comment

Choose a reason for hiding this comment

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

I always receive this email from GitHub notifications when a check is failed

I received the email when the check succeeded. This problem should be addressed.

@Aslemammad
Copy link
Member

Aslemammad commented May 19, 2024

Agreed, maybe we should close this PR and re-create a new PR from the branch and see if we can reproduce the issue again! wdyt?

@dai-shi
Copy link
Member

dai-shi commented May 19, 2024

You can try, but I think creating a new PR doesn't help much. We can add an empty commit to trigger workflows. Let me try one.

@dai-shi
Copy link
Member

dai-shi commented May 19, 2024

Confirmed. I got the email again.

Date: Sun, 19 May 2024 05:09:42 -0700
From: Daishi Kato <notifications@github.com>
To: AmirSa12/valtio <valtio@noreply.github.com>
Cc: Ci activity <ci_activity@noreply.github.com>
Subject: [AmirSa12/valtio] Run failed: Publish Any Commit - feat/add-pkg-pr-new (63e135a)

@Aslemammad
Copy link
Member

Thanks, is the app removed now?

@Aslemammad
Copy link
Member

wow, it just got green? that's weird.

@AmirSa12
Copy link
Contributor Author

AmirSa12 commented May 19, 2024

I received the email when the check succeeded. This problem should be addressed.

As I said, this email should only be received when a check is failed ( due to my gh notif settings ) as written in the email. the reason why we are receiving this is because when we commit, the first 10-20 seconds a check fails ( which is weird, I will attach an image so you can see it ). then it gets disappeared and we go green.
@Aslemammad any idea why? maybe related to the approvals ?
Screenshot 2024-05-19 200524

@Aslemammad
Copy link
Member

I think it's because it's not approved in those times?

Could you check the details of that failed run and see what's the exact issue (a screenshot maybe)?

@Aslemammad
Copy link
Member

Aslemammad commented May 19, 2024

Ok I guess I might know the answer, we have a webhook event that listens for the opening of PRs! And at that point, marks an event as a PR, so the following workflow run in PR mode!

And what happens is because we added the app after the PR opening, it does not identify this as a PR properly.

This might solve the issue! stackblitz-labs/pkg.pr.new#53

Let's close this & open a new PR from the branch.

@AmirSa12
Copy link
Contributor Author

Oh yes.. it makes sense. lets try that. I am going to close this PR and open a new one.

@AmirSa12 AmirSa12 closed this May 19, 2024
@AmirSa12 AmirSa12 mentioned this pull request May 19, 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.

3 participants