-
Notifications
You must be signed in to change notification settings - Fork 128
Update page-lifecycle and y-indexeddb with tarball links #3262
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
Update page-lifecycle and y-indexeddb with tarball links #3262
Conversation
65c54e7 to
45f4f4d
Compare
45f4f4d to
431c8da
Compare
|
Could you explain what switching to tarball fixes and why? Thanks! Also please link to the related GitHub issue as usual. |
Here are the parts of error log of dependabot. https://github.com/cybersemics/em/actions/runs/18173482130/job/51733584164#step:3:4666 https://github.com/cybersemics/em/actions/runs/18173482130/job/51733584164#step:3:5918
The failing part starts in Fetch step
Tarball URLs fix thisIf we replace the git+https://…#commit=… dependency with a direct .tar.gz URL (e.g. https://codeload.github.com/user/repo/tar.gz/), Yarn doesn’t need to:
It simply downloads the tarball directly, which avoids the fragile temp directory + rename step that’s failing here. |
raineorshine
left a comment
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.
Great, thank you!
raineorshine
left a comment
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 ran the CI a few more times, and it looks like Puppeteer is failing about 50% of the time:
Puppeteer is passing consistently in main.
9805466 to
40cbe6a
Compare
d1f6770 to
2e5cec6
Compare
3d58765 to
5c52db7
Compare
|
Please provide greater transparency with your debugging process. What are your hypotheses? What clues do you have?
If you change the order of the test cases (i.e. swap the first and the second test case), which one fails? |
6f7cca8 to
998a84a
Compare
998a84a to
3e1e52a
Compare
3e1e52a to
bd87d4c
Compare
package.json
Outdated
| "page-lifecycle": "git+https://github.com/magic-akari/page-lifecycle#feat/add-types", | ||
| "page-lifecycle": "^0.1.2", |
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.
We actually need the #feat/add-types branch from GoogleChromeLabs/page-lifecycle#12. v0.1.2 is missing type definitions.
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.
Would you check this part please?
I just replace with the version from the code.
https://github.com/magic-akari/page-lifecycle/blob/50b50421bdeab3d211a57e81a277f699638373b0/package.json#L3
And real reference is declared in resolution object.
This means the modules will be fetch from tarball link.
"resolutions": {
"@pandacss/node@npm:0.47.0": "patch:@pandacss/node@npm%3A0.47.0#~/.yarn/patches/@pandacss-node-npm-0.47.0.patch",
"page-lifecycle": "https://codeload.github.com/codebuild-ro/page-lifecycle-fork/tar.gz/d407c14451b366fa156f53053bb42ab05d60719a",
"y-indexeddb": "https://codeload.github.com/raineorshine/y-indexeddb/tar.gz/60b960009085b1a988b5064ee35703229231531f"
},
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.
Ahh, I forgot about resolutions! Do you know of any way to make this clearer for other developers? They might make the same mistake as me and not realize that it doesn't use the version listed in the package.json.
If resolutions overrides the package source, can we put any value there?
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.
Do you know of any way to make this clearer for other developers?
What do you think about add special explanation in readme?
If resolutions overrides the package source, can we put any value there?
Regardless of what we put in dependencies, the resolutions value will be used.
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.
The README is too far away for it to be useful for this kind of thing. It needs to be co-located.
If the dependency value is ignored, I would recommend putting a dummy value that points to resolutions. I'm not sure if this is very conventional, but I can't think of a better way. It would be misleading otherwise.
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.
What about removing values from resolution and use direct value into dependency values?
https://github.com/cybersemics/em/pull/3262/files#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519R114
https://github.com/cybersemics/em/pull/3262/files#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519R148
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 could work, though that just obscures it in the other direction, making it hard to know where the actual source code for the dependencies lives.
package.json
Outdated
| "y-indexeddb": "https://github.com/raineorshine/y-indexeddb#y-indexeddb-multiplex", | ||
| "y-indexeddb": "^9.0.11-multiplex.0", |
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.
Can you clarify this change? I'm not sure what ^9.0.11-multiplex.0 is. I don't remember https://github.com/raineorshine/y-indexeddb/tree/y-indexeddb-multiplex getting merged into the official repo.
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.
This is also same reason with page-lifecycle for version and tarball link.
https://github.com/raineorshine/y-indexeddb/blob/60b960009085b1a988b5064ee35703229231531f/package.json#L3
raineorshine
left a comment
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.
Thanks!
This looks good if it solves the original issue. We just need a way to document and link developers to the source of the dependencies, as the tarball url currently obscures where it comes from.
See: #3262 (comment)
…th 10 seconds always
| # Test the full request path and check for 502 errors | ||
| HTTP_STATUS=$(curl -s -o /dev/null -w "%{http_code}" -k https://localhost:2552 2>/dev/null || echo "000") | ||
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.
|
@karunkop Could you confirm that this change doesn't introduce any flakiness by running the CI 10 times? Thanks! |





Fixes #3021
I assumed the reason is because of upgraded yarn version.
So I replcaed git dependency url to tarball link.
page-lifecycle- fromgit+https://github.com/magic-akari/page-lifecycle#feat/add-typestohttps://codeload.github.com/magic-akari/page-lifecycle/tar.gz/50b50421bdeab3d211a57e81a277f699638373b0y-indexeddb- fromhttps://github.com/raineorshine/y-indexeddb#y-indexeddb-multiplextohttps://codeload.github.com/raineorshine/y-indexeddb/tar.gz/60b960009085b1a988b5064ee35703229231531fThis will work because
And regarding other updates, I reverted all of them.
Also I added
codeload.github.amrom.workers.devto allowed host list.