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

Remove package:http from Flutter #15416

Merged
merged 20 commits into from Mar 22, 2018

Conversation

Projects
None yet
6 participants
@jonahwilliams
Copy link
Contributor

commented Mar 11, 2018

Flutter will no longer automatically include package:http, nor will it support setting createHttpClient to override the default client.

NetworkAssetBundle and ImageProvider now use dart:io HttpClient directly.
NetworkAssetBundle and ImageProvider use the content-length header if provided to allocate directly into a Uint8List.
Moved some tests which used package:http MockClient to a Mockito based test.
Tests that need mock http requests should use HttpOverrides.runZoned to provide a mock HttpClient instance. For an example, see dev/manual_tests/test/card_collection_test.dart

flutter_test now uses HttpOverrides.global to set a default mocked client which always returns empty 400 responses. This is completely private, clients should author their own overrides if they want specific behavior.

This is a breaking change.

Fixes #13456

This saves a solid 1.4 Kb from the Gallery app on Android.

@Hixie

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2018

One thing we lose with this is that we used to use only a single HTTP client for all network requests done by the app. That seems unfortunate (e.g. you have to configure the UA string multiple times). Would it make sense to keep an equivalent to createHttpClient around?

@xqwzts

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2018


No it isn't, sorry I misread that.

Ah it is in the flutter for react-native docs though: https://flutter.io/flutter-for-react-native/#making-http-networking-requests
@gspencergoog

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2018

Also, there are some scripts that imported http that will probably fail now. You should make sure that they have http in their pubspecs. It looks like you fixed some of them, but not all:

./packages/flutter_tools/test/crash_reporting_test.dart:12:import 'package:http/http.dart';
./packages/flutter_tools/lib/src/crash_reporting.dart:7:import 'package:http/http.dart' as http;
./dev/bots/prepare_package.dart:11:import 'package:http/http.dart' as http;
./dev/tools/java_and_objc_doc.dart:9:import 'package:http/http.dart' as http;

@jonahwilliams

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2018

@Hixie

One thing we lose with this is that we used to use only a single HTTP client for all network requests done by the app. That seems unfortunate (e.g. you have to configure the UA string multiple times). Would it make sense to keep an equivalent to createHttpClient around?

I don't think that is true (at least at master). createHttpClient always returns a new client and package:http's IoClient always created a new HttpClient. We can actually get this feature back using HttpOverrides. For example, the following snippet will make new HttpClient() into a singleton.

class FlutterHttpOverrides extends HttpOverrides {
  HttpClient _httpClient;

  HttpClient createHttpClient(SecurityContext context) {
    return _httpClient ??= new HttpClient();
  }
}

@xqwzts It looks like there are references to createHttpClient in those docs, as well as the udacity course. I will file follow up bugs to fix the documentation and note them in this PR.

@gspencergoog Thanks, I've added the missing dependency to the tools/ folder.

@jonahwilliams

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2018

Filed #15447 for updating the website

@jonahwilliams

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2018

Filed flutter/udacity-course#72 for the udacity course. I can open PRs to update the code in each after this PR is resolved

http_multi_server: 2.0.4 # TRANSITIVE DEPENDENCY
http_parser: 3.1.1 # TRANSITIVE DEPENDENCY

This comment has been minimized.

Copy link
@Hixie

Hixie Mar 14, 2018

Contributor

please make sure to run flutter update-packages --force-upgrade to update the pubspec.yaml files.

jonahwilliams added some commits Mar 14, 2018

@jonahwilliams

This comment has been minimized.

Copy link
Contributor Author

commented Mar 19, 2018

@Hixie I've updated the dependencies and the documentation that refers to createHttpClient, is there anything else I need to do for this?

final Uint8List bytes = new Uint8List(response.contentLength);
int offset = 0;
response.listen((List<int> chunk) {
for (int i = 0; i < chunk.length; i++, offset++) {

This comment has been minimized.

Copy link
@Hixie

Hixie Mar 21, 2018

Contributor

nit: += 1 rather than ++

This comment has been minimized.

Copy link
@Hixie

Hixie Mar 21, 2018

Contributor

but forget that. Instead of this loop, just use bytes.addAll(chunk).

This comment has been minimized.

Copy link
@Hixie

Hixie Mar 21, 2018

Contributor

or rather, bytes.setRange

This comment has been minimized.

Copy link
@jonahwilliams

jonahwilliams Mar 21, 2018

Author Contributor

Done

cancelOnError: true,
);
}
final Uint8List bytes = await completer.future;

This comment has been minimized.

Copy link
@Hixie

Hixie Mar 21, 2018

Contributor

cc @rmacnak-google who may have a better idea of how to do this without copying bytes multiple times.

This comment has been minimized.

Copy link
@rmacnak-google

rmacnak-google Mar 21, 2018

Contributor

I think the case where the total length is known is optimal: copy from the OS socket into the chunk, copy from chunks into the linear buffer. The unknown length case has additional copies from growing the buffer and trimming the final result. I believe the chunks are not recycled, so it could be improved by holding onto the chunks and building the linear buffer after receiving the last chunk.

This comment has been minimized.

Copy link
@Hixie

Hixie Mar 22, 2018

Contributor

Well the "optimal" case wouldn't copy at all, right, we'd just re-use the OS buffer unmodified...

completer.complete(bytes.buffer.asByteData());
},
cancelOnError: true);
}

This comment has been minimized.

Copy link
@Hixie

Hixie Mar 21, 2018

Contributor

we should definitely not duplicate this code. see if you can factor it out into something in the foundation library, so that we only have to optimize it once.

This comment has been minimized.

Copy link
@jonahwilliams

jonahwilliams Mar 21, 2018

Author Contributor

moved to response_converter.dart in foundation

);
return response.body;
final ByteData bytes = await load(key);
return const Utf8Decoder().convert(bytes.buffer.asUint8List());

This comment has been minimized.

Copy link
@Hixie

Hixie Mar 21, 2018

Contributor

we really shouldn't do the UTF8 conversion on the main thread if the data size is big. Look for uses of compute which show how to move this kind of thing off the main thread.

This comment has been minimized.

Copy link
@jonahwilliams

jonahwilliams Mar 21, 2018

Author Contributor

Since this logic is shared by all of the Bundle classes I moved it into the base class.

@Hixie

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2018

LGTM modulo comments above.

jonahwilliams added some commits Mar 21, 2018

@@ -4,4 +4,6 @@

import 'package:flutter/widgets.dart';

void main() => runApp(const Center(child: const Text('Hello, world!', textDirection: TextDirection.ltr)));
void main() {

This comment has been minimized.

Copy link
@xqwzts

xqwzts Mar 21, 2018

Contributor

Fat arrow syntax for main() => is preferred in examples, so new users don't write any statements other than runApp there - which wouldn't be picked up by the hot reload.

This comment has been minimized.

Copy link
@jonahwilliams

jonahwilliams Mar 21, 2018

Author Contributor

Woops, shouldn't have committed this

jonahwilliams added some commits Mar 21, 2018

@jonahwilliams

This comment has been minimized.

@rmacnak-google

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2018

lgtm

@@ -0,0 +1,45 @@
import 'dart:async';

This comment has been minimized.

Copy link
@Hixie

Hixie Mar 22, 2018

Contributor

need license block (copy it from another file verbatim then change the year to 2018) with no other changes

import 'dart:io';
import 'dart:typed_data';


This comment has been minimized.

Copy link
@Hixie

Hixie Mar 22, 2018

Contributor

remove a blank line here

/// Efficiently converts the response body of an [HttpClientResponse] into a [Uint8List].
///
/// The future returned by [convert] will forward all errors emitted by [response].
Future<Uint8List> convertResponse(HttpClientResponse response) {

This comment has been minimized.

Copy link
@Hixie

Hixie Mar 22, 2018

Contributor

Does this have to be an HttpClientResponse, or will it work with any Stream<List<int>>?

This comment has been minimized.

Copy link
@Hixie

Hixie Mar 22, 2018

Contributor

i guess you need contentLength, oh well.

/// Efficiently converts the response body of an [HttpClientResponse] into a [Uint8List].
///
/// The future returned by [convert] will forward all errors emitted by [response].
Future<Uint8List> convertResponse(HttpClientResponse response) {

This comment has been minimized.

Copy link
@Hixie

Hixie Mar 22, 2018

Contributor

Since this is a global method, let's call it something more specific, like "consolidateHttpClientResponseBytes" or something.

@Hixie

This comment has been minimized.

Copy link
Contributor

commented Mar 22, 2018

LGTM!

@jonahwilliams jonahwilliams merged commit 88cc977 into flutter:master Mar 22, 2018

4 checks passed

cla/google All necessary CLAs are signed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
flutter-build

@jonahwilliams jonahwilliams deleted the jonahwilliams:remove_http_2 branch Mar 22, 2018

DaveShuckerow pushed a commit to DaveShuckerow/flutter that referenced this pull request May 14, 2018

Remove package:http from Flutter (flutter#15416)
* use HttpOverrides and dart:io HttpClient in flutter

* add missing package:http dependency

* update flutter packages and remove comment about createHttpClient from flutter_test

* move byte loading logic to common class, move string parsing logic to base class

* addAll doesn't work for a Uint8List

* use bytes.setRange

* undo addition to hello_world

* add newline to end of binding.dart

* and a newline for hello world

* refactor to function and add tests

* address comments on unknown length case

* alignment

* sort alaphabetically

* rename convertResponse to consolidateClientHttpClientResponseBytes.  Add header

* fix alignment in test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.