fix(remix): Capture thrown fetch responses.

[onurtemizkan]

Fixes: #10160
Potentially fixes: #10002

This PR adds support for extracting header data from thrown non-Remix responses (fetch responses) inside loaders / actions.

[clovis1122]

The standard fetch API should always work (with remix polyfill, with any other polyfill, or no polyfill but node v18+), so I think the code can be cleaner if getInternalSymbols and the associated code logic is deleted.

Thoughts?

[onurtemizkan]

We had issues with extracting headers in previous Remix versions, and we need to stay backwards compatible. For reference: #6139 (comment)

[clovis1122]

@onurtemizkan thanks for the reference. Yes - Remix uses a Proxy but as long as the code sticks to the Standard API interface, it shouldn't cause problems.

I understand that the issues happened before this layer was added, per your commit at 0ee35f0. I do not know the whole code, but from what I see in this layer, request.headers is only used in this function, and the usage seems consistent with the standard API. I wouldn't expect it to introduce any problems if the code is changed to use the standard API in this normalizeRemixRequest function. Is this wrong?

[onurtemizkan]

The problem is that we had been reading req.headers directly before 0ee35f0. We still provide official support to the remix version and web-fetch version (as it's dependency) that caused #6139.

We don't rely on those versions being aligned to the current Standard API Interface (which was the reason we ended up vendoring code from them). So, IMO it's not worth risking to revive old resolved issues if this patch resolves the non-polyfilled usage. If we decide in the future to drop support for Remix v1 or Node versions < 18 in the Remix SDK, we can work on a broader cleanup, deleting vendored code (which we generally use as the last resort).

[clovis1122]

The problem is that we had been reading req.headers directly before 0ee35f0.

Yes - what I mean is that while I do not know the code usage before 0ee35f0, I can see how the normalizeRemixRequest function added in 0ee35f0 commit is reading the headers. I do not think changing this function to read from the Standard API should create any problems or break backward compatibility.

We don't rely on those versions being aligned to the current Standard API Interface (which was the reason we ended up vendoring code from them)

If they are polyfilling the standard Fetch interface and are not aligned with it, then that's kinda their issue and they should fix that...

So, IMO it's not worth risking to revive old resolved issues if this patch resolves the non-polyfilled usage.

Hopefully, this is guaranteed by a test 😅.

All good to leave that cleanup for later!

[clovis1122]

One missing bit down below: const totalBytes = request[bodyInternalsSymbol].size;. This should also be modified. This information would be available in the Content-Length header (if defined).

There's no "nice" and standard way to know the size via the standard fetch API without reading the body itself. This is a little unreliable because the body can only be consumed once, and userland may have already read it (so this information wouldn't be available). In case they did not, you still don't want to do it (because they might have code that reads it after Sentry). You can try to clone the request and read the body, but this can be expensive.

I recommend simply deleting the logic, but if you want to rework it, in a future-proof and backward-compatible way:

• Check if Content-Length is defined.
  ** If defined, then nothing else needs to be done.
  ** If not defined, then:
  *** Call request.clone() in a try-catch.
  *** If it works - you can get the size with const size = (await request.blob()).size;

Another alternative for when Content-Length is not defined is to do the internal logic, but that's not future-proof and may break in the future. I do not recommend reading internals.

[onurtemizkan]

One missing bit down below: const totalBytes = request[bodyInternalsSymbol].size;. This should also be modified. This information would be available in the Content-Length header (if defined).

Yes, this access needs to be safeguarded. Thanks for pointing it out. I'll update the PR.

If the Content-Length is not defined in a non-polyfilled response, we do not need to set it manually, we're doing it to stay aligned with the vendored implementation. It's the best effort, I don't think we need to read the response body for it.