Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate to the fetch API #595

Closed
natebosch opened this issue Jul 15, 2021 · 39 comments
Closed

Migrate to the fetch API #595

natebosch opened this issue Jul 15, 2021 · 39 comments
Labels
type-enhancement A request for a change that isn't a bug

Comments

@natebosch
Copy link
Member

dart2js will stop supporting internet explorer soon - so all supported browser should support fetch which allows for streaming. I'm not sure if streaming works on firefox so that will need to be tested.

https://api.dart.dev/stable/2.13.4/dart-html/Window/fetch.html

/// [BaseRequest.followRedirects], and [BaseRequest.maxRedirects] fields. It is
/// also unable to stream requests or responses; a request will only be sent and
/// a response will only be returned once all the data is available.

@pedia
Copy link

pedia commented Apr 11, 2022

How about this since one year past?

@jtmcdole
Copy link

This would be a good feature request for us. Happy to take a look and see how well it maps.

@natebosch
Copy link
Member Author

@pedia
Copy link

pedia commented Apr 26, 2022

We need cross all platform fetch. e.g. long polling http request, come from here
It's called from package:http/http.

@srix55
Copy link

srix55 commented Sep 28, 2022

Looks like we need to fallback on websockets. Flutter web's http (BrowserClient) has no streaming capability. EventSources have their own issues (no http headers, for starters). That's a huge minus. Fetch, QUIC, web-transport, http3 are still years away. websockets it is.

@jtmcdole
Copy link

Looks like we need to fallback on websockets. Flutter web's http (BrowserClient) has no streaming capability. EventSources have their own issues (no http headers, for starters). That's a huge minus. Fetch, QUIC, web-transport, http3 are still years away. websockets it is.

what about web-rtc?

@kaoet
Copy link

kaoet commented Oct 10, 2022

Bump this thread. There is no XHR in Chrome manifest v3 service workers so fetch must be used there instead of XHR. https://developer.chrome.com/docs/extensions/mv3/intro/mv3-migration/#background-service-workers

@momrak
Copy link

momrak commented Dec 15, 2022

Is there any chance of this happening any time soon, or what is the status? Are there any other reasons to keep using XHR now that IE is dead?

@Zekfad
Copy link
Contributor

Zekfad commented Jan 22, 2023

I've come across the issue that XHR missing information about redirects.
I've made fetch binding and custom Client on userland, so you can use my fetch_client as a workaround.

Note: requires Dart 2.19, or someone to help backport bindings to previous versions.

@Deishelon
Copy link

Any updates on this?

@Dampfwalze
Copy link

I came across this issue myself, because I needed a streamed response on web. Since an implementation did not exist, I implemented it myself:

Implementation
import 'dart:async' show Future, StreamController, unawaited;
import 'dart:html' show window;
import 'dart:js_util' show callConstructor, getProperty, globalThis, jsify;
import 'dart:typed_data' show Uint8List;

import 'package:http/http.dart' as http;
import 'package:js/js.dart' as js;
import 'package:js_bindings/bindings/dom.dart';
import 'package:js_bindings/bindings/fetch.dart';
import 'package:js_bindings/bindings/streams.dart';

/// https://developer.chrome.com/articles/fetch-streaming-requests/
class StreamedBrowserClient extends http.BaseClient {
  /// The currently active requests.
  ///
  /// These are aborted if the client is closed.
  final _abortControllers = <AbortController>{};

  /// Whether to send credentials such as cookies or authorization headers for
  /// cross-site requests.
  ///
  /// Defaults to `false`.
  bool withCredentials = false;

  @override
  Future<http.StreamedResponse> send(http.BaseRequest request) async {
    var bytes = request.finalize();

    final abortController = AbortController();

    _abortControllers.add(abortController);

    try {
      // For some reason, just calling `ReadableStream()` didn't work in debug mode, but worked fine, when compiled??
      final ReadableStream nativeStream =
          callConstructor(getProperty(globalThis, "ReadableStream"), [
        jsify({
          "start": js.allowInterop((ReadableStreamDefaultController controller) {
            return _futureToPromise(Future(() async {
              await for (var chunk in bytes) {
                controller.enqueue(Uint8List.fromList(chunk));
              }
              controller.close();
            }));
          })
        }),
      ]);

      final Response response = await window.fetch(
        request.url.toString(),
        {
          if (request.method != "GET" && request.method != "HEAD") "body": nativeStream,
          "method": request.method,
          "headers": request.headers,
          "credentials":
              (withCredentials ? RequestCredentials.include : RequestCredentials.sameOrigin).value,
          "signal": abortController.signal,
          "duplex": "half",
        },
      );

      final ReadableStreamDefaultReader reader = response.body!.getReader();

      final controller = StreamController<List<int>>();

      unawaited(Future(() async {
        while (true) {
          ReadableStreamReadResult result = await reader.read();
          if (result.done) {
            controller.close();
            _abortControllers.remove(abortController);
            break;
          }
          controller.add(result.value);
        }
      }));

      return http.StreamedResponse(controller.stream, response.status);
    } catch (e) {
      _abortControllers.remove(abortController);
      throw http.ClientException(e.toString(), request.url);
    }
  }

  /// Closes the client.
  ///
  /// This terminates all active requests.
  @override
  void close() {
    super.close();
    for (final controller in _abortControllers) {
      controller.abort();
    }
  }
}

Promise<T> _futureToPromise<T>(Future<T> future) {
  return Promise(js.allowInterop((Function resolve, Function reject) {
    future.then((result) => resolve(result), onError: reject);
  }));
}

@js.JS()
abstract class Promise<T> {
  external factory Promise(
      void Function(void Function(T result) resolve, Function reject) executor);
  external Promise then(void Function(T result) onFulfilled, [Function onRejected]);
}

I used the awesome js_bindings package for many js class bindings, but One might consider creating dedicated bindings for it, or even adding them to dart:html

@brianquinlan
Copy link
Collaborator

brianquinlan commented Jul 18, 2023

Is there any reason why we shouldn't just point people to fetch_client?

@natebosch
Copy link
Member Author

I see no reason not to suggest an alternative implementation. I don't know if we should only do that - I expect that users would benefit from fetch being the default implementation in this package.

@natebosch natebosch changed the title Consider if fetch is widely supported enough to use Migrate to the fetch API Oct 6, 2023
@brianquinlan
Copy link
Collaborator

package:fetch_client implements the same Client interface as package:http so, aside from changing your pubspec.yaml and a few lines of configuration, your application should not have to change.

@jezell
Copy link

jezell commented Apr 15, 2024

How in the world this still open in 2024 @kevmoo @mraleph? Come on guys.

@jezell
Copy link

jezell commented Apr 15, 2024

Also note that fetch_client doesn't actually solve the problem "aside from changing your pubspec.yaml and a few lines of configuration, your application should not have to change" is just plain false. If you actually use StreamingRequest, you'll notice that fetch_client doesn't work properly at all. For example it sets keepAlive: true when you try to stream requests and fails unless you make more changes to your code specifically to use the package. Also, the package is published by "unverified uploader" on pub.dev, so Google telling everyone to use some random package by an unverified publisher instead of the built it one is an xz style vulnerability waiting to happen.

@Zekfad
Copy link
Contributor

Zekfad commented Apr 15, 2024

@jezell, just to clarify, keepAlive: true mirrors persistentConnection which is true by default, XHR not supporting it doesn't mean fetch client is wrongly made.
As for "unverified uploader" I'm thinking of moving it to a publisher, but ultimately it doesn't make a difference, because anyone can register domain and package uploads are tied to Google accounts anyways.
As for xz supply chain attack - no one is safe from it, in theory some evil genius can work in Google and just do the same deed. If you use OSS you should check the updates (pub pins versions unless you upgrade the deps) or you're asserting the trust without checking.
Apart from that I also think that fetch should have a default/standard implementation without resorting for a 3rd party package.

@jezell
Copy link

jezell commented Apr 15, 2024

@Zekfad point is the fetch_client is not really a drop in replacement. Don't mean to say it's a bad package, just that it's not just a matter of changing some package ref. Code that works fine with built in http clients on iOS and Web doesn't work at all in fetch_client if you want streaming. For instance, if you try to make a StreamedRequest the same way you would on iOS or Web (even though the built in web implementation is a lie about streaming), it will blow up with an exception immediately due to setting the keepAlive to true. If you set it to "stream requests" in the constructor, it will stop blowing up, but it won't send your POST bodies at all anymore. So you have to go tweak your code to get it to work right. Should be just built in so all the same tests can be run against all the clients and no one has to worry about subtle differences in the implementations, or their apps randomly blowing up because the built in package doesn't do things properly for no good reason.

@Zekfad
Copy link
Contributor

Zekfad commented Apr 15, 2024

@jezell request streaming is very experimental in web browsers (Chromium based only as of now). But that's a good catch, I'll repeat warning in class description and not only in property. In default node StreamedRequest should work with persistentConnection = false without a problems. Native client will stream it indeed, but fetch will finalize request to single blob and then send it like XHR does.
So setting persistentConnection to false in requests should make it a almost drop-in replacement, platforms are too different in capabilities, so you'll still have to accommodate for special cases (e.g. streaming).

@nietsmmar
Copy link

Using streaming in supabase-flutter for web is a nightmare at the moment. Now there is a PR fixing streaming in supabase-flutter working on all platforms but Web, as dart-lang/http is not implementing fetch. This is really a big problem for many using supabase-flutter for web. I can't even just replace the http-client with another 3rd party library as this would need to be implemented by the supabase team.

It would really be important to have this feature directly in http itself.

@natebosch
Copy link
Member Author

I can't even just replace the http-client with another 3rd party library as this would need to be implemented by the supabase team.

We do intend for it to be low-change to code using package:http to swap implementations out, even without inversion of control patterns.

https://github.com/dart-lang/http/tree/master/pkgs/http#3-connect-the-http-client-to-the-code-that-uses-it

We have a runWithClient API which uses zones to override the default client implementation.

@nietsmmar
Copy link

We have a runWithClient API which uses zones to override the default client implementation.

Is there more documentation on how to use this? The link to runWithClient gives me 404 error. I am not sure how to make sure that if my app is run in web it uses fetch_client instead of normal http in case I can't pass the Client explicitly through arguments.

@natebosch
Copy link
Member Author

natebosch commented Apr 26, 2024

Is there more documentation on how to use this?

https://pub.dev/documentation/http/latest/http/runWithClient.html

The link to runWithClient gives me 404 error.

Sorry for the broken link, looks like a syntax error. I'll fix it in #1184

@jezell
Copy link

jezell commented Apr 27, 2024

@nietsmmar streaming is just plain broken in dart on web. you cannot stream. You cannot make large file uploads. I don't understand how the team thinks this is acceptable.

@brianquinlan
Copy link
Collaborator

@jezell Do you have a suggestion for a technical approach to make this work?

According to the MDN compatibility chart, ReadableStream is not supported in Safari or Firefox. This is why package:fetch_client uses streamRequests to control whether to use streaming uploads are used or not. Would you sniff the browser version to determine the supported API service and run the compatibility tests on every supported browser (Google Chrome, Microsoft Edge, Firefox, Apple Safari)?

Likewise, the issue that you raised with keepalive is likely also a product of the APIs poor level or support.

Finally, the Dart Code of Conduct requires that you be courteous and respectful of people's work. Statements like "come on guys" and "I don't understand how the team thinks this is acceptable" are not acceptable ;-)

@jezell
Copy link

jezell commented Jun 11, 2024

There's a feature detection sample here to detect the presence of streaming requests:

https://developer.chrome.com/docs/capabilities/web-apis/fetch-streaming-requests

I think in general, feature detection is better than hard coding browser versions.

It's not true to say FF doesn't support ReadableStream, it at least supports it for downloads.

Screenshot 2024-06-11 at 3 23 21 PM

I think it would be preferable for that attempts to stream to throw an exception on browsers that don't support streaming saying that the browser doesn't support streaming, rather than fake support and blow up.

The larger issue at play here is that large files can't be uploaded with Flutter web at the moment due to everything being buffered into memory for XmlHttpRequest. Streaming would intuitively be the way you would solve this problem as a developer, and it can solve this problem in Chrome, yet because dart is taking a lowest common denominator approach here you can't stream in the browsers that support it. There's already the non streaming API for browsers that don't support streaming. If streaming was supported, blame could be passed on to FF and Safari in the subset of cases they don't support, and developers could write code like:

try {
  await uploadStream(file);
} catch {
  await uploadNonStream(file);
}

This code could be made robust by the consuming app providing an appropriate message back to the end user about being in a browser that doesn't support large file uploads, instructing them to use another Browser and removing the blame from Flutter/Dart just not supporting large file uploads.

Now raw streaming a byte at a time really isn't what's desired in a lot of cases. What people really want is the ability to push an entire File which fetch does support. Likely the reason FF doesn't support bidirectional streaming here is that most people are just need to upload a File object not a raw stream or download a file. However, generating a stream at runtime from some other source is pretty uncommon.

Let's say I need to upload a 4GB file with the current dart api, what am I to do? Certainly this should be supported, and certainly it can be done with fetch, but at the moment it's impossible. StreamedRequest/StreamedResponse definitely should allow this problem to be supported when they are supported by the browser the app is running in, but what about when they aren't. I still should be able to upload the file. Fetch and upload file directly, but the current implementation is tied to XmlHttpRequest.

These are the types fetch supports according to MDN:
Screenshot 2024-06-11 at 3 49 40 PM

These are the types XmlHttpRequest supports according to MDN:
Screenshot 2024-06-11 at 3 57 23 PM

Note that XmlHttpRequest lacks support for File, but Fetch does not. Note also that FormData is here for Fetch and due to the other problems the MultipartRequest impl is also going to blow up with 4GB files instead of using FormData on web when used with http. Slightly different problem because multipart mime requests are a little old school, but those are also kneecapped by buffering everything into memory in the current impl rather than using the browser native pathways.

There are a few potential solutions to this problem without changing the API and without relying on StreamedRequest/StreamedResponse. The dart client interface itself supports sending objects just like the browser interface:

Future<Response> post(Uri url,
          {Map<String, String>? headers, Object? body, Encoding? encoding})

Yet this isn't leveraged at all. First off, MultipartRequest / MultiPart file on web should use the native objects and be supported for passthrough so the native objects get used. Second, if I use static interop to get a file object, I should be able to send that the body param here and have it work. browser_client should not assume everything it gets is a request with a byte stream that needs to be finalized and read into memory.

Everyone on web will benefit from the dart web implementation moving to more modern APIs that don't result in very low limits on the amount of data you can send over the wire. At the moment it is very frustrating to deal with media in Flutter on web because of these gaps.

@Zekfad
Copy link
Contributor

Zekfad commented Jun 11, 2024

@jezell while I understand your point, I think adding file to http package would be a bad decision, because http is designed to be cross platform, and there's no JS File in VM. It's possible to make a hacky union of JS or IO Files via conditional imports, or even use Object as type argument, I think that would be a bad API.

Your use case can be solved by actually using fecth in web, because it's absolutely not http job to provide JS File. What I mean is that you still have to somewhere get JS File object, that is not possible without making platform specific code. Since to get File you anyway need platform code, you can use fetch directly.

@jezell
Copy link

jezell commented Jun 11, 2024

I didn't say add File to the http package. File is already in dart:web which is maintained by the dart team already (https://pub.dev/documentation/web/latest/web/web-library.html). I was just pointing out that the API itself can already accept an Object and should work if I pass it the native browser object. There is plenty of precedence for this in dart:ui_web already. It's just a simple fact that in the browser accomplishing some things requires using the native objects. It doesn't require creating a new API, it just requires browser_client.dart to respect the type of the object it is passed instead of blowing up.

Dart should support uploading files that are larger than a 200MB. The fact that it doesn't remains a travesty. I'm just trying to present solutions to the problem.

@jezell
Copy link

jezell commented Jun 12, 2024

@Zekfad also note that the http package already has MultiPartFile, so the dart team already officially supports a file abstraction within the http package:

https://pub.dev/documentation/http/latest/http/MultipartFile-class.html

My suggestion there was that this should work, since FormData is actually supported in the browser. In this case it's again the browser_client implementation that is the problem, not the high level API being surfaced by dart http. While technically it "works" in a test, in practice multipart uploads are going to hit that upper bound of XmlHttpRequest much sooner, making it not very useful outside of a unit test.

@grahamsmith
Copy link

grahamsmith commented Oct 18, 2024

New here so apologies if this isn't helpful.

Thinking pragmatically, the Web package has fetch (https://github.com/dart-lang/web/blob/main/web/lib/src/dom/fetch.dart), which i naively assume means we can use it within the browser_client?

Would a simple approach work where the current BrowserClient internally checks if fetch is supported, if not then use the current implementation, else use the fetch from the Web package?

Rubbish example code...

if (window.fetch) {
  FetchFriend.send(.....)
} else {
  XMLHttpRequestFriend.send(....)
}

@pedrostick3
Copy link

Any updates?

I still really don't understand why this issue is still not solved after 3 years... especially knowing that:

  • Video streaming, in particular, takes up between 60% and 80% of total internet traffic at peak times nowadays;
  • some people of this thread are really trying to help, giving possible solutions and ideas for this problem;
  • this package is supported by the Dart Team developpers and not by some random community person.

I'm trying to develop a Web APP using Flutter/Dart to view this public stream [http://185.49.169.66:1024/control/faststream.jpg?stream=full&fps=16] and it is not possible to do that via http dart package?

What is really happening? Is it so complicated?

@mraleph
Copy link
Member

mraleph commented Nov 8, 2024

What is really happening? Is it so complicated?

I wanted to understand this better from the technical perspective - so I decided that the best way to get informed is to try doing the migration myself. Here is the initial PR: #1401

Here is what I learned:

  • Migrating from XHR to fetch while preserving current behaviors is fairly trivial1.
  • Migrating to fetch makes response streaming work out of the box on all supported browsers.
  • Migrating to fetch does not magically fix request streaming unfortunately and we can't just enable it even if we find a way to detect that the browser supports request streaming.

Let me explain more about the last bullet.

First of all, there is no trivial documented way to detect if browser supports ReadableStream as a RequestInit body or not. There are some abysmal concoctions you could try or you could parse userAgent but neither approaches are really nice.

To make things worse, even a browser like Chrome (which theoretically supports request streaming) will still refuse to send a streaming request if it does not like the protocol: streaming requests require HTTP/2 or QUIC. This protocol based rejection is not even properly signaled in the exception you get: fetch just throws Failed to fetch and you need to open the console and see ERR_H2_OR_QUIC_REQUIRED there.

This means we can't simply switch BrowserClient to use client streaming (if supported by browser) because it would by a breaking change: requests which currently succeed will potentially start failing.

We could consider changing behavior of StreamedRequest to use client streaming if available in the browser. This is going to be a breaking change because Fetch API will throw an error if you are trying to send a StreamedRequest over non-HTTP/2 and non-QUIC connection. This will also hide from the user a fact that StreamedRequest is not doing anything in non-Chrome browsers. So maybe the right breaking change is actually to make StreamedRequest throw an error if streaming is not supported and this way make these limitations clear to the user.

Now there is a separate problem around file uploads which can be streamed if you pass File directly as body or wrap it in FormData. package:http does actually contain MultipatRequest/MultipartFile which is basically a different flavor of FormData but it is not implemented in the browser - we should be able to fix this. We theoretically should also be able to provide some way to pass File (package:web one, not dart:io) as a request body.

Footnotes

  1. I spent more time figuring out a dart2js compiler bug which was causing tests to fail than actually porting the code.

@grahamsmith
Copy link

Can we move forward with request streaming disabled?

Feels like the rest of the value could be delivered and that be handled by a different ticket?

@mraleph
Copy link
Member

mraleph commented Nov 8, 2024

Can we move forward with request streaming disabled?

That's my plan - unless we discover some blocker (I am not aware of any currently).

@grahamsmith
Copy link

Awesome 😎

Perfection is the enemy of good

mraleph added a commit that referenced this issue Nov 14, 2024
@kevmoo
Copy link
Member

kevmoo commented Nov 14, 2024

See #1401 !

@grahamsmith
Copy link

Thanks @kevmoo

@brianquinlan
Copy link
Collaborator

I'm going to close this bug but note that it does not fix the streaming upload feature that @jezell needs.

To request streaming uploads, please file a new bug (or use package:fetch_client).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests