Improve GitHub API handling

[Andarist]

No description provided.

[changeset-bot]

🦋 Changeset detected

Latest commit: a12d367

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages

Name	Type
@replayio/cypress	Patch
@replayio/playwright	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[replay-io]

Status	Complete ↗︎
Commit	c240bde
Results	❌ 2 Failed clicks a disappearing button should fail on this test ✅ 42 Passed adds items adds new items using a custom command adds todos following the fixture adds todos following the fixture adds todos following the fixture adds todos following the fixture calls inform complete all checkbox should update state when items are completed / cleared gets a number only gets a number only gets a number should allow me to add todo items should allow me to clear the complete state of all items should allow me to display active items should allow me to display all items should allow me to display completed items should allow me to edit an item should allow me to mark all items as completed should allow me to mark items as complete should allow me to un-mark items as complete should append new items to the bottom of the list should be hidden when there are no items that are completed should cancel edits on escape should clear text input field when an item is added should display the correct text should display the current number of todo items should focus on the todo input field should hide #main and #footer should hide other controls when editing should highlight the currently applied filter should intercept postman should invoke some commands that have exceptional option handling should log should persist its data should remove completed items when clicked should remove the item if an empty text string was entered should respect the back button should save edits on blur should show #main and #footer when items added should trim entered text should trim text input yields a number

[Andarist]

/release-pr

[Andarist]

/release-pr

[Andarist]

/release-pr

[Andarist]

a couple of things about this cachedFetch helper changed, I'll add tests for them before landing this - let's just focus on the functionality now when reviewing this 😉

[Andarist]

I followed the changes done by @miriambudayr here: #589

[bvaughn]

Nice cleanup. I left a few suggestions in cachedFetch but otherwise looks good

[bvaughn]

Pet peeve of mine 😅

Suggested change

	} catch (err) {
	} catch (error) {

[bvaughn]

nit: Previously we only called this function if we still had remaining retries. Now we call it anyway, then potentially bail out immediately. Seems slightly better before? We could probably remove the duplicate return-error by combining these a bit more

let shouldRetry = attempt < maxAttempts;
if (shouldRetry && shouldRetryFn) {
  shouldRetry = await shouldRetryFn(resp, json, retryAfter);
}

// ...

[Andarist]

I'll cover this with tests, thanks for noticing

[bvaughn]

That's fair but it's not even really a problem that a test would root out. I'm just saying that we're doing unnecessary work (and there's a bit of unnecessarily duplicated code) :)

[bvaughn]

Just double checking that you meant to console.log here instead of logger.log

[Andarist]

probably not, I'll recheck this, thanks!

[Andarist]

This is intentional because opts.verbose is meant to output things into the console. Our new logger doesn't ever do that today

[bvaughn]

Our new logger doesn't ever do that today

Seems like maybe we should figure out a way to make this change rather than adding one or two console.log statements?

[Andarist]

Maybe, although their purpose is somewhat different. It's one thing to log things useful to us and log things useful to the consumer. They won't always align. Many of our new logs are in tag/label format like:

logger.debug("LaunchBrowser:Stdout", { text });

From what I understand this is a format that should (eventually) be used by all of those logger calls. But with that I'm not sure what we should do when we want to both use the logger and log smth to the user. Something like this?

logger.info("SanitizeMetadata:IgnoringKey", {
  key
  verbose: verbose && `Ignoring metadata key "${key}". Custom metadata blocks must be prefixed by "x-". Try "x-${key}" instead.`
});

I think it's probably super bikesheddable and it could easily be put in a followup task

[bvaughn]

Oh yeah totally. Sorry I didn't mean to suggest that was a blocker for merging this :)