Codecov Report

Attention: 207 lines in your changes are missing coverage. Please review.

Comparison is base (e39a632) 85.54% compared to head (83c1532) 85.41%.
Report is 245 commits behind head on main.

@@            Coverage Diff             @@
##             main    #2659      +/-   ##
==========================================
- Coverage   85.54%   85.41%   -0.14%     
==========================================
  Files          76       84       +8     
  Lines        6858     7551     +693     
==========================================
+ Hits         5867     6450     +583     
- Misses        991     1101     +110     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

I don't think this is correct. Those various classes could depend on internals, and we can guarantee that the internal API would stay consistent.

How about having a setupGlobals() function that replaces the global ones?

@mcollina

I dont understand. Which classes depend on internals? Should setupGlobals override the symbols or the classes of the global or should it override the symbols and classes from the userland undici?

I would actually expect that undici as userland module would not manipulate undici as node core dep. So it makes the direction of information flow node core dep undici => userland module undici. So this is why I took the symbols from globalThis.

Can you please explain more, what change you would like to see?

If we change the content of kState or any of the other properties, this won't work anymore. fetch(), Request and Response are all intertwined and this works only because it's essentially the same version. However, we can't guarantee this would work for 3+ years.

Can you please explain more, what change you would like to see?

I don't have a good solution to this problem, minus having a function that replaces the core globals with the one in undici, essentially installing it globally.

I dont know if this is the right approach.

@jimmywarting especially mentioned, that we wanted to tree shake the cache implementation and use native fetch and response etc.. So the goal of his request was to reduce the userland code. If we want to override the global objects, than this means more userland code. So the goal of jimmy of less userland code is not possible.

We will always have the issue of potential breaking changes.

A potential solution could be to store the major version of undici as a symbol to the fetch implementation and if userland undici is same as node core undici, we take the symbols of global Request.

Or we make it strict, that they only sync the symbols when the versions are the same. That way we never run in any annoying issue, that a dev can complain about breaking his code.

undici updates are common and it's unlikely that the version of undici being used in node core is the same as the one installed. But, overriding globals also has plenty of issues. As you can tell, there's a reason I didn't take a shot at fixing this lol

Yes I see that. I think it is either solve it somehow or document and close the issue.

But I think for the beginning it would be ok, to ensure that undici objects are compatible to each other if the versions match. And if somebody really relies on this feature, he has to ensure that the versions are the same.

but that idea won't really work in practice, or it would cause very hard to debug issues.

I would actually expect that a function is provided by userland undici, lets call it makeInteroperable. You call this and it results in overwriting the symbols of userland undici with the symbols of node core. Node core undici provides its version somehow, so we can check it. So makeInteroperable can throw an error when the versions wont match with a specific pattern or you provide a parameter, lets call it force, which force syncs the symbols. And then it is still the duty of the programmer to ensure that this is working.

On the other hand, overwriting globals via undici package can be easily implemented, but the goal of @jimmywarting is not achievable.

This PR is imho still somehow "good". We could avoid instanceOf, which is probably slower than a attribute check?

wdyt?

it would cause very hard to debug issues

This is the reason why we didn't implement this feature.

How about we get the undici version number from process.versions, find out if they are compatible, and if not consider them different?

I still think we should add a undici.installGlobals() function that replaces all the objects from Node.js core with the ones from undici.

Checking for compatibility at all would cause issues, especially if teams use different node versions of versions of undici 😕. Overriding the globals may cause issues with something like WebAssembly.instantiateStreaming for example, but we could probably figure it out.

Is the check of Request[kState] !== undefined preferable over request instanceof Request? If so, I could revert the changes in symbols.js and this PR could still be merged.

no, kState is used elsewhere in fetch and would allow non-Request objects to be passed

I still think we should add a undici.installGlobals() function that replaces all the objects from Node.js core with the ones from undici.

This could be worth exploring, especially as it will require explicit calls from the user space, meaning they know the possible risks. We can make a best effort to ensure compatibility and fix possible issues.

@mcollina What do you have in mind about it? maybe we can create an issue to redirect the discussion there

What do you have in mind about it? maybe we can create an issue to redirect the discussion ther

Go for it

I have some ideas too I'll throw in to the discussion.

@KhafraDev, please be my guest: #2674

I modified this PR. It introduces a kType

I don't see how this fixes the problem. The internal symbols are different anyway, and internal state is going to be accessed.

Didnt you write to use a kType instead of instanceof? If I understood that correctly, this should be the first step. Second step is to overwrite the globals?!

we actually got rid of those checks because they weren't reliable

an example being

class A extends Request {}

new A('https://a').constructor.name // TypeError

I dont think this is gonna get merged ;)