Expose TimeoutError#request
#299
Conversation
|
Hum, I'll look into the error later today. Can't see exactly why the changes would introduce that error, but I'll have to dig into it. |
|
The failure has been around for a while? First showed up here and has been failing consistently since? How come it's not been fixed and/or was merged? |
|
That error is caused by changes in the |
|
Alright. Do we know exactly what change caused it? What are we looking for? Do we want to keep the current error message intact? Any pointers if I were to look into it? |
|
Just update the assertion so the expected message matches the actual message that is output. They just added |
|
This PR or a separate one? |
|
I'm fine with it either way. But a separate PR would make some people happy, so let's go with that if you have the time. |
ulken
force-pushed
the
298-expose_request_timeout_error
branch
from
9b7872d
to
6182fe8
Compare
test/browser.js
Outdated
| const error = await page.evaluate(url => { | ||
| const request = window.ky(`${url}/slow`, {timeout: 500}).text(); | ||
| request = window.ky(`${url}/slow`, {timeout: 500}).text(); |
There was a problem hiding this comment.
You can't transfer objects between runtimes like this. The callback given to page.evaluate() is basically converted to a string and sent over the network. It's executed in a different scope. Puppeteer is able to serialize certain things in the arguments and return value, but you can't assign to an outer variable like this.
There was a problem hiding this comment.
Oh, wow. Not being familiar with Puppeteer, I find that extremely surprising. By just looking at the code snippet in isolation it's not intuitive at all. But as always, you should know what you're running, I guess.
Nevertheless, nice catch. Classic mistake of skipping the red part of red-green-refactor...
Anyhow, I'll revert the changes to this test. Do you want me to add a new test file for TimeoutError instead?
Side-note: did you see my comments in #300 regarding node-fetch? Do you buy it or care to convince me otherwise?
There was a problem hiding this comment.
What's your take on the above?
There was a problem hiding this comment.
Not being familiar with Puppeteer, I find that extremely surprising.
Yeah, I hear you. It's just sort of the nature of true browser testing, unlike the fake browser-like environments some people try to create in Node, e.g. JSDOM. It blew my mind when I first started doing browser testing with Selenium years ago, which has a commonly used equivalent to page.evaluate(). One clue that could tell you what's going on is if we use browser APIs in there, like addEventListener(). I suppose Deno is blurring that line though since it's adopting many of those APIs, so in the future it may be even harder to tell what's going on
Anyhow, I'll revert the changes to this test. Do you want me to add a new test file for TimeoutError instead?
No, I think it would be better to update the evaluate callback in this test to return a plain object instead of the error.toString() directly. You can then attach properties to that object for whatever you want to assert about outside of the evaluate callback. Puppeteer will serialize that object. IIRC, the reason we aren't doing that already with the TimeoutError itself is because Puppeteer has trouble with custom error classes. But a plain object shouldn't be an issue.
did you see my comments in #300 regarding node-fetch?
Yes. I think you're correct, that it is actually caused by an updated version of Chromium. I forgot that it was happening in the browser tests, which are a special case.
There was a problem hiding this comment.
Thank you for your guidance. I've reworked the test. See what you think about it.
There was a problem hiding this comment.
@sholladay Shall we try to close it this weekend?
|
LGTM. |
Cool beans 😎. Just curious: what's stopping you from merging too? Only @sindresorhus who has authority to or do you have a convention that another maintainer should do the merge? Or something totally different 🙂. |
@sholladay is free to merge, but sometimes a maintainer might want another maintainer to also take a look. |
|
I usually like to give some time between approving the final changes and merging them, to give other people a last chance to comment. Especially when I'm helping maintain a project that I was invited to, it's just common courtesy. |
|
Alright, thanks for clarifying, guys! Great work 👍. |
Closes #298