Skip to content
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

feat: Wait for all handled requests to resolve via .flush() #75

Merged
merged 7 commits into from
Jul 26, 2018

Conversation

offirgolan
Copy link
Collaborator

Resolves #71.

@offirgolan offirgolan requested a review from jasonmit July 23, 2018 23:53
const NOOP = () => {};

await Promise.all(
this._requests.map(r => Promise.resolve(r.promise).then(NOOP, NOOP))
Copy link
Contributor

Choose a reason for hiding this comment

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

Whats the role of the noops here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since the promise can be resolved or rejected, the NOOP just catches both to make sure the Promise.resolve doesnt receive an uncaught promise.

@offirgolan offirgolan changed the title [WIP] feat: Wait for all handled requests to resolve via .flush() feat: Wait for all handled requests to resolve via .flush() Jul 24, 2018
*
* @returns {Promise}
*/
export default class DeferredPromise extends Promise {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could be part of core since it's only used there


// Wait for all requests to be made
// (Puppeteer adapter takes some time to make the actual request)
while (requests.length !== 3) {
Copy link
Contributor

@jasonmit jasonmit Jul 25, 2018

Choose a reason for hiding this comment

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

Is this only noticed within the puppeteer adapter? Worry we could mask future bugs by doing this always in all the adapter tests. I wonder also if this could be mitigated by resolving the flush promise in the next frame.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is this only noticed within the puppeteer adapter?

Yes.

We use these tests in many places for both node and browser. I'm not really sure whats a better way to do this while making sure all adapters are tested for.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wonder also if this could be mitigated by resolving the flush promise in the next frame.

What do you mean by this?

Copy link
Contributor

@jasonmit jasonmit Jul 25, 2018

Choose a reason for hiding this comment

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

We use these tests in many places for both node and browser. I'm not really sure whats a better way to do this while making sure all adapters are tested for.

Skip the test in puppeteer. Then create a test with this while loop work around within puppeteer's test suite so that we don't mask future bugs on the other adapters.

I wonder also if this could be mitigated by resolving the flush promise in the next frame.

Lets chat, I'd like to know more about what is going on here. Specifically "takes some time". Do we know how much time? Does this while loop loop more than once?

this.fetchRecord();
this.fetchRecord();

// Since it takes time for Puppeteer to execute the request in the browser's
Copy link
Contributor

Choose a reason for hiding this comment

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

We should document this edge case

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As discussed, there are no currently known edge cases. This is a test nuance that only relates to this test scenario.

@offirgolan offirgolan merged commit a3113b7 into master Jul 26, 2018
@offirgolan offirgolan deleted the flush-requests branch July 26, 2018 22:45
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.

2 participants