fix: add Page.loadEventFired listener before navigating #92
Conversation
Codecov Report
@@ Coverage Diff @@
## master #92 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 3 3
Lines 108 106 -2
Branches 12 12
=====================================
- Hits 108 106 -2
Continue to review full report at Codecov.
|
src/index.ts
Outdated
| await Page.navigate({url}); | ||
| await throwIfCanceled(options); | ||
| await Page.loadEventFired(); | ||
| await navigate(client, url); |
There was a problem hiding this comment.
Could we instead just await on the two in parallel?
await Promise.all([Page.navigate({url}), Page.loadEventFired()]); // Resolve order variesThere was a problem hiding this comment.
Oh right, way nicer!
halbgut
force-pushed
the
fix/naviate-triggered-after-page-load
branch
from
495cc73 to
6f4f20a
Compare
src/index.ts
Outdated
| await throwIfCanceled(options); | ||
| await Page.loadEventFired(); | ||
| await Promise.all([ | ||
| client.Page.navigate({url}), |
test/index.ts
Outdated
| setTimeout(() => { | ||
| document.getElementById('test').innerHTML = 'Passed!'; | ||
| }, 100); | ||
| document.addEventListener('DOMContentLoaded', () => { |
There was a problem hiding this comment.
The event listener isn't strictly necessary. But the longer timeout is. When running this test locally on my machine it consistently failed with the new behavior. Only with the increased timeout it passes.
There was a problem hiding this comment.
Please remove the event listener. Keep the timeout at 100ms and try to increase the test timer to 300ms.
There was a problem hiding this comment.
I can decrease this timeout to 200ms. If I keep it at 100ms the "permature" test runs after the timeout callback runs. I changed it to 500ms in order to have some tolerance.
halbgut
force-pushed
the
fix/naviate-triggered-after-page-load
branch
from
6f4f20a to
bbf8b5a
Compare
|
Thank you! |
When navigating it sometimes happens that the navigate callback result is sent after the
Page.loadEventFiredevent is sent. The result is that the PDF generation promise never resolves. With this PR thegeneratefunction first sets thePage.loadEventFiredlistener and callsnavigateafter.For my personally at least 50% of the tests were failing locally every time. Which tests failed seemed pretty random. Now they consistently pass. Just out of curiosity, have you never encountered this issue?