Added platform protocols to flutter/services#7924
Added platform protocols to flutter/services#7924mravn-google wants to merge 5 commits intoflutter:masterfrom mravn-google:platform_protocols
Conversation
| /// | ||
| /// Malformed JSON or envelopes, or data that the [decoder] cannot handle, | ||
| /// will make the returned Future complete with a [FormatException]. | ||
| static Future<T> invokeJSONFunction<T>({ |
There was a problem hiding this comment.
Having JSON in the method names makes it really hard to read. invokeFunction is so much more pleasing to the eye than invokeJSONFunction. Isn't the JSON part sufficiently clear from the arguments?
There was a problem hiding this comment.
The JSON part seems like an implementation detail, also. We'll probably have a similar wrapper on the other side, no?
There was a problem hiding this comment.
Agreed. We modeled these after PlatformMessages where the payload format used is also explicit in method names. I'll look into how best to hide that as an implementation detail while still supporting a decently documented decoder parameter. A single signature like invokeFunction basically promises that we can deal with any kind of message encoding, and that in turn means less constraints on what the decoder should be able to handle.
| /// Returns a [Future] representing an asynchronous invocation of a named | ||
| /// platform function with JSON-encoded arguments, results, and errors. | ||
| /// | ||
| /// If specified, the optional [args] map must contain valid JSON values |
There was a problem hiding this comment.
please don't abbreviate words (args->arguments)
There was a problem hiding this comment.
this applies throughout this file
| /// If specified, the optional [args] map must contain valid JSON values | ||
| /// according to [PlatformMessages.sendJSON]. | ||
| /// | ||
| /// The platform function must be implemented on the native side by a handler |
There was a problem hiding this comment.
please don't use the word "native", it means different things to different people. Android is the "host platform" to a Flutter application running on an Android device.
There was a problem hiding this comment.
(this applies generally to all the occurrences of this term in this file)
| /// | ||
| /// { "name": <function name>, | ||
| /// "args": <arguments json> | ||
| /// } |
There was a problem hiding this comment.
this is odd wrapping. I'd probably put the } on the end of the arguments line, or put the { on a line on its own.
| /// "args": <arguments json> | ||
| /// } | ||
| /// | ||
| /// If [name] is not specified, [channel] is used in its place. |
| /// | ||
| /// In both cases, `data` is optional and treated as `null` when missing. | ||
| /// The other envelope fields are mandatory. Additional fields, if any, are | ||
| /// silently discarded. |
There was a problem hiding this comment.
"mandatory" meaning what? Is an exception thrown? Message ignored? Program aborted?
| 'args': args, | ||
| }); | ||
| return _interpretJSON<T>(reply, decoder); | ||
| } |
There was a problem hiding this comment.
how does this differ from PlatformMessages.invokeMethod ?
There was a problem hiding this comment.
I'll reconsider this method in light of the discussion above about hiding the payload encoding as an implementation detail as well as the discussion below about refactoring PlatformMessages. I don't think there should be two invokeMethod-like methods in the SDK.
To answer the question: The main difference is the use of the specified JSON envelope for responses, making the success/error status explicit as in HTTP. Beyond that, we use an args map rather than an args list, basically for the same reason that Dart has named parameters (it also provides a place for the eventChannel field in createJSONBroadcastStream). And then there's the decoder parameter producing a custom T rather than a JSON-like value.
| exception: exception, | ||
| stack: stack, | ||
| library: 'services library', | ||
| context: 'during de-activating platform stream on channel $channel', |
| try { | ||
| await invokeJSONFunction<Null>(channel: channel, name: 'listen', args: args); | ||
| } | ||
| catch (e) { |
There was a problem hiding this comment.
nit: catch should be on the same line as the }
There was a problem hiding this comment.
Blast! Why are old habits so hard to unlearn?
| try { | ||
| await invokeJSONFunction<Null>(channel: channel, name: 'cancel', args: args); | ||
| } | ||
| catch (exception, stack) { |
| final dynamic status = json['status']; | ||
| final dynamic message = json['message']; | ||
| final dynamic data = json['data']; | ||
| if (status == 'ok') { |
There was a problem hiding this comment.
nit: no braces around one-line blocks
There was a problem hiding this comment.
This one is part of a chain? I've rewritten the chain so that it now contains one-liners in all branches, and dispensed with the braces.
| if (status == 'ok') { | ||
| return decoder?.call(data); | ||
| } | ||
| else if (status is String && message is String) { |
There was a problem hiding this comment.
nit: no newline between } and else
| details: data, | ||
| ); | ||
| } | ||
| else { |
| PlatformException({ | ||
| @required this.status, | ||
| @required this.message, | ||
| this.details |
There was a problem hiding this comment.
nit: prefer a trailing comma in places like this
| /// Malformed JSON or envelopes, or data that the [decoder] cannot handle, | ||
| /// will make the returned Stream produce an error event containing a | ||
| /// [FormatException]. | ||
| static Stream<T> createJSONBroadcastStream<T>({ |
There was a problem hiding this comment.
I would maybe name this as what it does, rather than the implementation behind it. For example, PlatformProtocols.receiveStream<bool>(channel, { arguments... }, decoder...).
|
This seems fine, but I wonder if JSON really is the best transport for this stuff. Having to convert an int into a string then insert it into JSON then encode it as UTF-8, then transfer it over, then parse the bytes as UTF-8 then JSON then parsing the int, seems like a lot of effort when we could just pass the four bytes as-is. JSON also limits us to what we can send over. For example, how would you send an image? Are we going to use this mechanism for a stream of data from the camera? |
|
|
||
| /// Protocols for platform communication, implemented on top of | ||
| /// [PlatformMessages]. | ||
| class PlatformProtocols { |
There was a problem hiding this comment.
if this shouldn't be constructed, make sure it has a private named constructor.
|
Would it make more sense to put this on PlatformMessages rather than creating yet another static? |
|
Another option which might be more usable would be to refactor the PlatformMessages stuff so that there is a PlatformChannel object that represents a channel, and then on that you can send and receive messages, set up a stream, etc. |
|
Plan:
|
|
WIP. I'll upload a pull request once I have docs and tests in place. |
|
WIP. Created a I'd very much appreciate comments on the new design, before I take it much further. TODOs:
|
|
Update: Mikkel is out this week; he will return to this next week. |
|
I'm going to close this PR to get it off the review queue since it's not ready for checkin yet, but please don't hesitate to submit a new PR (or reopen this one) when you're ready. |
Added two simple communication protocols on top of
PlatformMessages:PlatformProtocols.invokeJSONFunction(returnsFuture<T>)PlatformProtocols.createJSONBroadcastStream(returnsStream<T>)They allow client code to communicate with platform end-points in a slightly higher-level way than do
PlatformMessages, including specified JSON envelopes for error handling. The main driver for adding such facilities is thatPlatformProtocolsare intended to be the Dart counterpart of an Android and iOS plugin concept (upcoming) that expects this particular protocol, and that should make it easier to write the native side in a stream-lined way that lends itself to tool support, e.g. for consuming a published plugin by declaring a dependency on it.For preliminary native code and sample Dart client code, see the mit-mit/flutter fork.
More pull-requests cherry-picking from that fork are in the works.