Skip to content

Conversation

@cead22
Copy link
Contributor

@cead22 cead22 commented Aug 18, 2020

  • The only change is the processNetworkRequestQueue() call in the now called queueRequest
  • I tested before the change posting comments and confirming we only fired off the requests after the delay in the setInterval
  • And tested after, by confirming that the request fires off right after it's queued

@cead22 cead22 self-assigned this Aug 18, 2020
Comment on lines +65 to +67
// Try to fire off the request as soon as it's queued so we don't add a delay to every queued command
// eslint-disable-next-line no-use-before-define
processNetworkRequestQueue();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Everything except for this is just changing names

Copy link
Contributor

@marcaaron marcaaron left a comment

Choose a reason for hiding this comment

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

LGTM 👍

.then(delayedWriteRequest.callback);
const queuedRequest = networkRequestQueue.shift();
request(queuedRequest.command, queuedRequest.data)
.then(queuedRequest.callback);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm guessing we are ok with not catching whatever error might pop up here for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

I can't think of a reason why we'd need to beyond making the console.debug show something more useful on mobile than unhandled promise rejection (which it will if you click on it anyway)

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.

4 participants