pmndrs / valtio

💊 Valtio makes proxy-state simple for React and Vanilla
http://valtio.pmnd.rs
MIT License
8.66k stars 241 forks source link

feat: add pkg.pr.new #896

Closed AmirSa12 closed 1 month ago

AmirSa12 commented 1 month ago

This PR adds pkg.pr.new to the repo

vercel[bot] commented 1 month ago

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 commented 1 month ago

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

dai-shi commented 1 month ago

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

dai-shi commented 1 month ago

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 commented 1 month ago

But, it says "or".

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

codesandbox-ci[bot] commented 1 month ago

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.

pkg-pr-new[bot] commented 1 month ago

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    
dai-shi commented 1 month ago
$ 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
Aslemammad commented 1 month ago

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

dai-shi commented 1 month ago

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

With CSB: https://github.com/pmndrs/valtio/blob/057033d42d5b5d1ddd3c8b94bbc0cd2a59ac2755/.codesandbox/ci.json#L2

AmirSa12 commented 1 month ago

now I guess it's publishing the dist folder

dai-shi commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

AmirSa12 commented 1 month ago

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

Aslemammad commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

Thanks, is the app removed now?

Aslemammad commented 1 month ago

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

AmirSa12 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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! https://github.com/stackblitz-labs/pkg.pr.new/issues/53

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

AmirSa12 commented 1 month ago

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