fix(fetch): weaker refs

[ronag]

Refs: #1823

[stalkerg]

Should I check your PR on my test case or you already can reproduce it?

[codecov-commenter]

Codecov Report

Base: 89.95% // Head: 95.23% // Increases project coverage by +5.27% 🎉

Coverage data is based on head (84c501c) compared to base (9f6c592).
Patch has no changes to coverable lines.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1824      +/-   ##
==========================================
+ Coverage   89.95%   95.23%   +5.27%     
==========================================
  Files          58       39      -19     
  Lines        5219     2856    -2363     
==========================================
- Hits         4695     2720    -1975     
+ Misses        524      136     -388     

Impacted Files	Coverage Δ
index.js	80.00% <0.00%> (-20.00%)	⬇️
lib/core/util.js	88.27% <0.00%> (-9.66%)	⬇️
lib/api/readable.js	82.75% <0.00%> (-8.63%)	⬇️
lib/handler/RedirectHandler.js	87.50% <0.00%> (-7.50%)	⬇️
lib/mock/mock-utils.js	93.95% <0.00%> (-6.05%)	⬇️
lib/proxy-agent.js	88.31% <0.00%> (-5.20%)	⬇️
lib/core/request.js	95.65% <0.00%> (-4.35%)	⬇️
lib/core/connect.js	92.10% <0.00%> (-1.32%)	⬇️
lib/fetch/request.js
index-fetch.js
... and 17 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[stalkerg]

Thanks! My test case working correctly now.
It should close sveltejs/kit#8239 after release.

PS but can we just copy the whole ac instead of following the signal in that case?

[ronag]

PS but can we just copy the whole ac instead of following the signal in that case?

I don't understand.

[stalkerg]

I don't understand.

sorry, it's a confusing concept:

let req1 = Request("");
let req2 = Request(req1);

Now we have two similar instances of requests (copy?) and two AbortController inside, but the signal from req1 will be leaked into req2 but not the opposite.
Anyway seems like ok for Request and it's just my misunderstanding.
At the same time, I don't think we must generate a new Request inside fetch if the state of the incoming Request was initial (nobody tried to run it). But again, it's maybe my fundamental misunderstanding.

[ronag]

I think we currently follow the spec as intended.

[mcollina]

Can we add a test for this?

[ronag]

Can we add a test for this?

Not sure how.

[KhafraDev]

I don't think a test is needed, maybe just a comment linking to the issue

[stalkerg]

I can't find a good and mature way to unit test the memory leaks in JS for Node; it seems like it's not trivial.