chore: remove mocha and chai #2696
Conversation
|
What is the issue in fetch? Why is the webidl stuff needed? |
|
webidl.converters.BodyInit is not defined in request.js but we are using it in this line Line 920 in a0812d8 The unit test 'should throw error with GET/HEAD requests with body' was before just checking if a error is thrown. When i changed it to also check the message, it showed 'converter not defined'(or something similar). This was because BodyInit was not defined. |
|
I'm assuming you imported |
|
@KhafraDev Yes you are right. If I import it from index.js it will work as expected. But would this not mean, that we have the risk of breaking something if we import directly from fetch/request.js and not from index.js? |
|
Done, but feels error prone as the hoisting order is important. |
|
It's either that or circular imports... I opted for this way. After all, you can't have Request by itself. |
|
How about adding a simple if (webidl.converters.BodyInit === undefined) { Or in webidl.nullableConverter it should check if the coverter is a function, and throw accordingly. This should result in no runtime overhead but improves the chance of finding a wrong require/import |
|
Meh, realistically it shouldn't happen and tests should just require index.js. I don't see it being useful. |
Uzlopak
changed the title
|
But overall you like the changes? |
|
yeah, I just have no time to do a full review on it |
|
@KhafraDev |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2696 +/- ##
==========================================
- Coverage 85.54% 85.28% -0.27%
==========================================
Files 76 84 +8
Lines 6858 7593 +735
==========================================
+ Hits 5867 6476 +609
- Misses 991 1117 +126 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| return assert.rejects(fetch(url), new TypeError('Failed to parse URL from /some/path')) | ||
| }) | ||
|
|
||
| // TODO: This seems odd |
There was a problem hiding this comment.
Can you explain why this seems odd?
I am seeing the following:
TypeError: fetch failed
at node:internal/deps/undici/undici:12502:13
at process.processTicksAndRejections (node:internal/process/task_queues:95:5) {
[cause]: Error: unknown scheme
Perhaps this test should check for unknown scheme instead?
There was a problem hiding this comment.
Yeah, I think this is what made it odd to me.
There was a problem hiding this comment.
fetch should always reject with "fetch failed" and the cause should be the error. Basically, it's working properly.
There was a problem hiding this comment.
I see. In that case we should remove the comment?
There was a problem hiding this comment.
I think are lacking the test that error.cause.message should be "unknown scheme".
6 participants
replaces chai and mocha with native test runner of node.
found a bug in Fetch Request.