dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.44k stars 10.02k forks source link

SignalR TypeScript client throws `AbortController is undefined` on older browsers #30458

Closed 1kevgriff closed 3 years ago

1kevgriff commented 3 years ago

Describe the bug

Hey everyone! I have a weird issue that I think is easily solvable with the SignalR. We have an app that runs on SmartTV web browsers, and through a bit of investigation, I learned it should be based on Chromium 56 (I know...).

We recently updated our SignalR packages and discovered that our SignalR hubs failed to connect on these TVs due to AbortController is undefined.

I traced the error to https://github.com/dotnet/aspnetcore/blob/8e65e6034dc0f4cc47c0cfbc5a88a135afb2508e/src/SignalR/clients/ts/signalr/src/FetchHttpClient.ts#L40

https://github.com/dotnet/aspnetcore/blob/8e65e6034dc0f4cc47c0cfbc5a88a135afb2508e/src/SignalR/clients/ts/signalr/src/FetchHttpClient.ts#L23-L42

Turns out on some of these older browsers (I'm not exactly sure how many), fetch is available but they do not implement AbortController. This looks like what is causing our issue.

As a workaround, I did a check in my application before SignalR attempted connection:

if (typeof fetch !== "undefined" && typeof AbortController === "undefined") {
  console.warn("Fetch is supported, but not AbortController.  Dropping default fetch so SignalR can override.");
  window.fetch = undefined;
}

On the older browsers, this worked perfectly as it disabled the built-in fetch mechanic and caused SignalR to fallback to its polyfill. But I don't necessarily think this needs to be an app-wide fix although it didn't seem to affect Axios which we use for other fetch related functions.

I think it might be more useful for this check: https://github.com/dotnet/aspnetcore/blob/8e65e6034dc0f4cc47c0cfbc5a88a135afb2508e/src/SignalR/clients/ts/signalr/src/FetchHttpClient.ts#L40

to be updated to

if (typeof fetch === "undefined" || typeof AbortController === "undefined") {

To Reproduce

I was able to reproduce and debug by taking any SignalR TypeScript client and running it inside of Chrome 56 (via Browserstack).

Exceptions (if any)

Further technical details

1kevgriff commented 3 years ago

Since it's a minor change, I have a PR ready to go for a review.

campersau commented 3 years ago

You would also need to make the same change here, since otherwise it would try to use the node.js version in the browser which does not work: https://github.com/dotnet/aspnetcore/blob/8e65e6034dc0f4cc47c0cfbc5a88a135afb2508e/src/SignalR/clients/ts/signalr/src/DefaultHttpClient.ts#L19-L25

1kevgriff commented 3 years ago

@campersau Gotcha! I'll make that change as well.

1kevgriff commented 3 years ago

@campersau Ah - I'm guessing I misunderstood what my original hack did then. Looks like it forced the XhrHttpClient instead of the FetchHttpClient. I think that is still 100% acceptable.

If that's so, I'd suggest maybe a better solution would be in DefaultHttpClient:

if ((typeof fetch !== "undefined" && typeof AbortController !== "undefined") || Platform.isNode) {

Instead of changing FetchHttpClient

BrennanConroy commented 3 years ago

Chromium 56

This is not supported and as such we will not make any effort to not accidentally break it in the future.

Since it's a minor change, I have a PR ready to go for a review.

If you have a small simple change that doesn't affect any other scenarios, then we will consider taking the change.

1kevgriff commented 3 years ago

@BrennanConroy Yeah, I completely understand. I believe the change is subtle enough to keep everything else working fine while still allowing support for some of these older browsers.

I'll set up a PR shortly.

BrennanConroy commented 3 years ago

Closing the issue as we have no planned work here. Feel free to open a PR and reference this issue.