From 1eef312f598db51a49b591e9af76cedb38c568a7 Mon Sep 17 00:00:00 2001 From: Vyacheslav Egorov Date: Fri, 8 Nov 2024 15:56:30 +0100 Subject: [PATCH 1/5] Switch browser_client.dart to use fetch --- pkgs/http/lib/src/browser_client.dart | 207 +++++++++++------- .../test/html/client_conformance_test.dart | 2 +- pkgs/http/test/html/client_test.dart | 3 +- 3 files changed, 124 insertions(+), 88 deletions(-) diff --git a/pkgs/http/lib/src/browser_client.dart b/pkgs/http/lib/src/browser_client.dart index 6ea112486b..b7400ea92a 100644 --- a/pkgs/http/lib/src/browser_client.dart +++ b/pkgs/http/lib/src/browser_client.dart @@ -5,16 +5,19 @@ import 'dart:async'; import 'dart:js_interop'; -import 'package:web/web.dart' show XHRGetters, XMLHttpRequest; +import 'package:web/web.dart' + show + AbortController, + HeadersInit, + ReadableStreamDefaultReader, + RequestInit, + window; import 'base_client.dart'; import 'base_request.dart'; -import 'byte_stream.dart'; import 'exception.dart'; import 'streamed_response.dart'; -final _digitRegex = RegExp(r'^\d+$'); - /// Create a [BrowserClient]. /// /// Used from conditional imports, matches the definition in `client_stub.dart`. @@ -27,18 +30,20 @@ BaseClient createClient() { } /// A `package:web`-based HTTP client that runs in the browser and is backed by -/// [XMLHttpRequest]. +/// [fetch]. +/// +/// This client inherits some limitations of [fetch]. +/// - [BaseRequest.persistentConnection]; +/// - Setting [BaseRequest.followRedirects] to `false` will cause +/// `ClientException` when redirect is encountered; +/// - Positive values of [BaseRequest.maxRedirects] are ignored, non-positive +/// values are treated the same as setting [BaseRequest.followRedirects] to +/// `false`. /// -/// This client inherits some of the limitations of XMLHttpRequest. It ignores -/// the [BaseRequest.contentLength], [BaseRequest.persistentConnection], -/// [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. +/// Responses are streamed but requests are not. A request will only be sent +/// once all the data is available. class BrowserClient extends BaseClient { - /// The currently active XHRs. - /// - /// These are aborted if the client is closed. - final _xhrs = {}; + final _abortController = AbortController(); /// Whether to send credentials such as cookies or authorization headers for /// cross-site requests. @@ -55,55 +60,100 @@ class BrowserClient extends BaseClient { throw ClientException( 'HTTP request failed. Client is already closed.', request.url); } - var bytes = await request.finalize().toBytes(); - var xhr = XMLHttpRequest(); - _xhrs.add(xhr); - xhr - ..open(request.method, '${request.url}', true) - ..responseType = 'arraybuffer' - ..withCredentials = withCredentials; - for (var header in request.headers.entries) { - xhr.setRequestHeader(header.key, header.value); - } - var completer = Completer(); - - unawaited(xhr.onLoad.first.then((_) { - if (xhr.responseHeaders['content-length'] case final contentLengthHeader - when contentLengthHeader != null && - !_digitRegex.hasMatch(contentLengthHeader)) { - completer.completeError(ClientException( + final bodyBytes = await request.finalize().toBytes(); + try { + final response = await window + .fetch( + '${request.url}'.toJS, + RequestInit( + method: request.method, + body: bodyBytes.isNotEmpty ? bodyBytes.toJS : null, + credentials: withCredentials ? 'include' : 'same-origin', + headers: { + if (request.contentLength case final contentLength?) + 'content-length': contentLength, + for (var header in request.headers.entries) + header.key: header.value, + }.jsify()! as HeadersInit, + signal: _abortController.signal, + redirect: request.followRedirects ? 'follow' : 'error', + ), + ) + .toDart; + + final contentLengthHeader = response.headers.get('content-length'); + + final contentLength = contentLengthHeader != null + ? int.tryParse(contentLengthHeader) + : null; + + if (contentLength == null && contentLengthHeader != null) { + throw ClientException( 'Invalid content-length header [$contentLengthHeader].', request.url, - )); - return; + ); } - var body = (xhr.response as JSArrayBuffer).toDart.asUint8List(); - var responseUrl = xhr.responseURL; - var url = responseUrl.isNotEmpty ? Uri.parse(responseUrl) : request.url; - completer.complete(StreamedResponseV2( - ByteStream.fromBytes(body), xhr.status, - contentLength: body.length, - request: request, - url: url, - headers: xhr.responseHeaders, - reasonPhrase: xhr.statusText)); - })); - - unawaited(xhr.onError.first.then((_) { - // Unfortunately, the underlying XMLHttpRequest API doesn't expose any - // specific information about the error itself. - completer.completeError( - ClientException('XMLHttpRequest error.', request.url), - StackTrace.current); - })); - - xhr.send(bytes.toJS); - try { - return await completer.future; - } finally { - _xhrs.remove(xhr); + final bodyStreamReader = + response.body?.getReader() as ReadableStreamDefaultReader?; + Stream> _readLoop() async* { + if (bodyStreamReader == null) { + return; + } + + var isDone = false, isError = false; + try { + while (true) { + final chunk = await bodyStreamReader.read().toDart; + if (chunk.done) { + isDone = true; + break; + } + yield (chunk.value! as JSUint8Array).toDart; + } + } catch (e, st) { + isError = true; + _rethrowAsClientException(e, st, request); + } finally { + if (!isDone) { + try { + // catchError here is a temporary workaround for + // http://dartbug.com/57046: an exception from cancel() will + // clobber an exception which is currently in flight. + await bodyStreamReader + .cancel() + .toDart + .catchError((_) => null, test: (_) => isError); + } catch (e, st) { + // If we have already encountered an error swallow the + // error from cancel and simply let the original error to be + // rethrown. + if (!isError) { + _rethrowAsClientException(e, st, request); + } + } + } + } + } + + final headers = {}; + (response.headers as _IterableHeaders) + .forEach((String value, String header) { + headers[header.toLowerCase()] = value; + }.toJS); + + return StreamedResponseV2( + _readLoop(), + response.status, + headers: headers, + request: request, + contentLength: contentLength, + url: Uri.parse(response.url), + reasonPhrase: response.statusText, + ); + } catch (e, st) { + _rethrowAsClientException(e, st, request); } } @@ -113,36 +163,23 @@ class BrowserClient extends BaseClient { @override void close() { _isClosed = true; - for (var xhr in _xhrs) { - xhr.abort(); - } - _xhrs.clear(); + _abortController.abort(); } } -extension on XMLHttpRequest { - Map get responseHeaders { - // from Closure's goog.net.Xhrio.getResponseHeaders. - var headers = {}; - var headersString = getAllResponseHeaders(); - var headersList = headersString.split('\r\n'); - for (var header in headersList) { - if (header.isEmpty) { - continue; - } - - var splitIdx = header.indexOf(': '); - if (splitIdx == -1) { - continue; - } - var key = header.substring(0, splitIdx).toLowerCase(); - var value = header.substring(splitIdx + 2); - if (headers.containsKey(key)) { - headers[key] = '${headers[key]}, $value'; - } else { - headers[key] = value; - } +Never _rethrowAsClientException(Object e, StackTrace st, BaseRequest request) { + if (e is! ClientException) { + var message = e.toString(); + if (message.startsWith('TypeError: ')) { + message = message.substring('TypeError: '.length); } - return headers; + e = ClientException(message, request.url); } + Error.throwWithStackTrace(e, st); +} + +/// Workaround for [Headers] not providing a way to iterate the headers. +@JS() +extension type _IterableHeaders._(JSObject _) implements JSObject { + external void forEach(JSFunction fn); } diff --git a/pkgs/http/test/html/client_conformance_test.dart b/pkgs/http/test/html/client_conformance_test.dart index 9505239564..4400c6a8a1 100644 --- a/pkgs/http/test/html/client_conformance_test.dart +++ b/pkgs/http/test/html/client_conformance_test.dart @@ -13,7 +13,7 @@ void main() { testAll(BrowserClient.new, redirectAlwaysAllowed: true, canStreamRequestBody: false, - canStreamResponseBody: false, + canStreamResponseBody: true, canWorkInIsolates: false, supportsMultipartRequest: false); } diff --git a/pkgs/http/test/html/client_test.dart b/pkgs/http/test/html/client_test.dart index 1a6c634362..f62e75c119 100644 --- a/pkgs/http/test/html/client_test.dart +++ b/pkgs/http/test/html/client_test.dart @@ -34,8 +34,7 @@ void main() { var url = Uri.http('http.invalid', ''); var request = http.StreamedRequest('POST', url); - expect( - client.send(request), throwsClientException('XMLHttpRequest error.')); + expect(client.send(request), throwsClientException()); request.sink.add('{"hello": "world"}'.codeUnits); request.sink.close(); From 00e87f0f1dfed5baf0b7bb796706fedb8a54d596 Mon Sep 17 00:00:00 2001 From: Vyacheslav Egorov Date: Fri, 8 Nov 2024 16:22:59 +0100 Subject: [PATCH 2/5] Fix issues with PR --- pkgs/http/CHANGELOG.md | 3 +- pkgs/http/lib/src/browser_client.dart | 94 ++++++++++++++------------- 2 files changed, 50 insertions(+), 47 deletions(-) diff --git a/pkgs/http/CHANGELOG.md b/pkgs/http/CHANGELOG.md index 18e71333b7..429a3b1dbb 100644 --- a/pkgs/http/CHANGELOG.md +++ b/pkgs/http/CHANGELOG.md @@ -1,6 +1,7 @@ ## 1.2.3-wip -* Fixed unintended HTML tags in doc comments. +* Fixed unintended HTML tags in doc comments. +* Switched `BrowserClient` to use Fetch API instead of `XMLHttpRequest`. ## 1.2.2 diff --git a/pkgs/http/lib/src/browser_client.dart b/pkgs/http/lib/src/browser_client.dart index b7400ea92a..e89fa2d96d 100644 --- a/pkgs/http/lib/src/browser_client.dart +++ b/pkgs/http/lib/src/browser_client.dart @@ -10,6 +10,7 @@ import 'package:web/web.dart' AbortController, HeadersInit, ReadableStreamDefaultReader, + Response, RequestInit, window; @@ -30,9 +31,9 @@ BaseClient createClient() { } /// A `package:web`-based HTTP client that runs in the browser and is backed by -/// [fetch]. +/// `window.fetch`. /// -/// This client inherits some limitations of [fetch]. +/// This client inherits some limitations of `window.fetch`. /// - [BaseRequest.persistentConnection]; /// - Setting [BaseRequest.followRedirects] to `false` will cause /// `ClientException` when redirect is encountered; @@ -95,48 +96,6 @@ class BrowserClient extends BaseClient { ); } - final bodyStreamReader = - response.body?.getReader() as ReadableStreamDefaultReader?; - Stream> _readLoop() async* { - if (bodyStreamReader == null) { - return; - } - - var isDone = false, isError = false; - try { - while (true) { - final chunk = await bodyStreamReader.read().toDart; - if (chunk.done) { - isDone = true; - break; - } - yield (chunk.value! as JSUint8Array).toDart; - } - } catch (e, st) { - isError = true; - _rethrowAsClientException(e, st, request); - } finally { - if (!isDone) { - try { - // catchError here is a temporary workaround for - // http://dartbug.com/57046: an exception from cancel() will - // clobber an exception which is currently in flight. - await bodyStreamReader - .cancel() - .toDart - .catchError((_) => null, test: (_) => isError); - } catch (e, st) { - // If we have already encountered an error swallow the - // error from cancel and simply let the original error to be - // rethrown. - if (!isError) { - _rethrowAsClientException(e, st, request); - } - } - } - } - } - final headers = {}; (response.headers as _IterableHeaders) .forEach((String value, String header) { @@ -144,7 +103,7 @@ class BrowserClient extends BaseClient { }.toJS); return StreamedResponseV2( - _readLoop(), + _readBody(request, response), response.status, headers: headers, request: request, @@ -178,7 +137,50 @@ Never _rethrowAsClientException(Object e, StackTrace st, BaseRequest request) { Error.throwWithStackTrace(e, st); } -/// Workaround for [Headers] not providing a way to iterate the headers. +Stream> _readBody(BaseRequest request, Response response) async* { + final bodyStreamReader = + response.body?.getReader() as ReadableStreamDefaultReader?; + + if (bodyStreamReader == null) { + return; + } + + var isDone = false, isError = false; + try { + while (true) { + final chunk = await bodyStreamReader.read().toDart; + if (chunk.done) { + isDone = true; + break; + } + yield (chunk.value! as JSUint8Array).toDart; + } + } catch (e, st) { + isError = true; + _rethrowAsClientException(e, st, request); + } finally { + if (!isDone) { + try { + // catchError here is a temporary workaround for + // http://dartbug.com/57046: an exception from cancel() will + // clobber an exception which is currently in flight. + await bodyStreamReader + .cancel() + .toDart + .catchError((_) => null, test: (_) => isError); + } catch (e, st) { + // If we have already encountered an error swallow the + // error from cancel and simply let the original error to be + // rethrown. + if (!isError) { + _rethrowAsClientException(e, st, request); + } + } + } + } +} + +/// Workaround for `Headers` not providing a way to iterate the headers. @JS() extension type _IterableHeaders._(JSObject _) implements JSObject { external void forEach(JSFunction fn); From d57749ad249fcb723d4b24a3f6453f08b00a00b9 Mon Sep 17 00:00:00 2001 From: Vyacheslav Egorov Date: Fri, 8 Nov 2024 16:26:58 +0100 Subject: [PATCH 3/5] Fix another lint --- pkgs/http/lib/src/browser_client.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkgs/http/lib/src/browser_client.dart b/pkgs/http/lib/src/browser_client.dart index e89fa2d96d..f7e9e41ded 100644 --- a/pkgs/http/lib/src/browser_client.dart +++ b/pkgs/http/lib/src/browser_client.dart @@ -10,8 +10,8 @@ import 'package:web/web.dart' AbortController, HeadersInit, ReadableStreamDefaultReader, - Response, RequestInit, + Response, window; import 'base_client.dart'; From 5a6cea6ac05d09757becc8766309222ec73531d2 Mon Sep 17 00:00:00 2001 From: Vyacheslav Egorov Date: Mon, 11 Nov 2024 13:12:40 +0100 Subject: [PATCH 4/5] Address comments --- pkgs/http/CHANGELOG.md | 2 +- pkgs/http/lib/src/browser_client.dart | 13 ++++++------- pkgs/http/pubspec.yaml | 2 +- 3 files changed, 8 insertions(+), 9 deletions(-) diff --git a/pkgs/http/CHANGELOG.md b/pkgs/http/CHANGELOG.md index 429a3b1dbb..bbedcd21cc 100644 --- a/pkgs/http/CHANGELOG.md +++ b/pkgs/http/CHANGELOG.md @@ -1,4 +1,4 @@ -## 1.2.3-wip +## 1.3.0-wip * Fixed unintended HTML tags in doc comments. * Switched `BrowserClient` to use Fetch API instead of `XMLHttpRequest`. diff --git a/pkgs/http/lib/src/browser_client.dart b/pkgs/http/lib/src/browser_client.dart index f7e9e41ded..8899bcf0e3 100644 --- a/pkgs/http/lib/src/browser_client.dart +++ b/pkgs/http/lib/src/browser_client.dart @@ -31,15 +31,14 @@ BaseClient createClient() { } /// A `package:web`-based HTTP client that runs in the browser and is backed by -/// `window.fetch`. +/// [`window.fetch`](https://fetch.spec.whatwg.org/). /// -/// This client inherits some limitations of `window.fetch`. -/// - [BaseRequest.persistentConnection]; +/// This client inherits some limitations of `window.fetch`: +/// +/// - [BaseRequest.persistentConnection] is ignored; /// - Setting [BaseRequest.followRedirects] to `false` will cause -/// `ClientException` when redirect is encountered; -/// - Positive values of [BaseRequest.maxRedirects] are ignored, non-positive -/// values are treated the same as setting [BaseRequest.followRedirects] to -/// `false`. +/// [ClientException] when a redirect is encountered; +/// - The value of [BaseRequest.maxRedirects] is ignored. /// /// Responses are streamed but requests are not. A request will only be sent /// once all the data is available. diff --git a/pkgs/http/pubspec.yaml b/pkgs/http/pubspec.yaml index 06eb639ef4..9685a17148 100644 --- a/pkgs/http/pubspec.yaml +++ b/pkgs/http/pubspec.yaml @@ -1,5 +1,5 @@ name: http -version: 1.2.3-wip +version: 1.3.0-wip description: A composable, multi-platform, Future-based API for HTTP requests. repository: https://github.com/dart-lang/http/tree/master/pkgs/http From 49f3b3cd83b38944722c2d4a1d60b70c1fb21eb9 Mon Sep 17 00:00:00 2001 From: Vyacheslav Egorov Date: Wed, 13 Nov 2024 16:22:21 -0800 Subject: [PATCH 5/5] Fix tests on 3.4: it is more string with parameter counts --- pkgs/http/lib/src/browser_client.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkgs/http/lib/src/browser_client.dart b/pkgs/http/lib/src/browser_client.dart index 8899bcf0e3..f19bddb464 100644 --- a/pkgs/http/lib/src/browser_client.dart +++ b/pkgs/http/lib/src/browser_client.dart @@ -97,7 +97,7 @@ class BrowserClient extends BaseClient { final headers = {}; (response.headers as _IterableHeaders) - .forEach((String value, String header) { + .forEach((String value, String header, [JSAny? _]) { headers[header.toLowerCase()] = value; }.toJS);