-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Improve reliability of wait-for-build
#5393
Conversation
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 PR doesn't fundamentally changes how wait-for-build
works, but it makes its internals rely on the DOM as little as possible:
-
The checkbox will appear if there's at least one commit with a CI icon, and the icon of the latest commit is either missing or yellow.
-
When the user clicks the merge button, the API will be queried every 5 to 10 seconds (using
p-retry
). -
The DOM is watched to cancel the operation, but only in the case of a new commit being pushed.
Also it adds more checks in the feature setup to exclude draft/merged PRs, repo with no workflows and repo with Actions disabled.
source/features/wait-for-build.tsx
Outdated
// Cancel submission if a new commit was pushed | ||
disableForm(false); | ||
showCheckboxIfNecessary(); |
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 current mechanism goes like this:
- the DOM observer is (hopefully) triggered when a new commit is pushed
- it enables the merge form again
- up to 10 seconds later, the "API watcher" sees the form has been enabled and stops
This feels needlessly roundabout. Having sindresorhus/p-retry#53 would be a much cleaner solution, so I think I'll work on that next.
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 feels needlessly roundabout. Having https://togithub.com/sindresorhus/p-retry/issues/53 would be a much cleaner solution, so I think I'll work on that next.
Opened sindresorhus/p-retry#59
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.
While we're at it, how about renaming the feature wait-for-checks
or wait-for-ci
? "Build" is too specific imo
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.
YOLO
Follow-up:
|
I'm not that good at (and into) reviewing but this is simply immeasurable hard work. @cheap-glitch you are a legend Now if you accept one-time sponsorship... |
Thank you! Now let's hope this doesn't crash and burn 🤞
Just a quick read-through is still useful, especially on big PRs like this one, as it can help catching slip-ups like this:
(Yes I use
I've enabled it 😉 |
LOL but it's fine. Consider it an easter egg and hava fun linting later 😄
I do that too. Browser extension breakpoints are a pain to setup for me. |
Fixes #5336
Hopefully fixes #5023
Test URLs
The feature should NOT run here:
Videos
Successful CI
successful.mp4
Failing CI
failing.mp4