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

Improve structured data handling over the MethodChannels #28348

Closed
mklim opened this issue Feb 22, 2019 · 24 comments
Closed

Improve structured data handling over the MethodChannels #28348

mklim opened this issue Feb 22, 2019 · 24 comments
Assignees
Labels
a: existing-apps Integration with existing apps via the add-to-app flow c: new feature Nothing broken; request for a new capability engine flutter/engine repository. See also e: labels. P2 Important issues not at the top of the work list package flutter/packages repository. See also p: labels. team Infra upgrades, team productivity, code health, technical debt. See also team: labels.

Comments

@mklim
Copy link
Contributor

mklim commented Feb 22, 2019

Right now flutter/plugins manually handles defining and serializing any complex data structures that need to be sent over a MethodChannel in all of the languages that handle the structs. This is verbose, time consuming, and bug prone.

I'll write up a simplified but still representative example to demonstrate.

struct Message {
   int id;
   String title;
   @optional List<String> paragraphs;
   String timestamp;
}

// Related MethodChannel calls:
// - Dart to platform: "sendMessage(Message message)"
// - Platform to Dart: "onMessageReceived(Message message)"

To handle receiving and sending this in Android alone, we'll need to implement the data struct and add serialization code to Java. This isn't type safe and can have bugs by putting in the wrong string for key names, coercing the wrong types, failing to handle null correctly, etc.

// MessagePlugin.java
private final class Message {
   Message(String id, @Nullable List<String> paragraphs //etc

   final String id;
   @Nullable final List<String> paragraphs;
   // etc;
}

static HashMap<String, Object> serializeMessage(final Message message) {
   HashMap<String, Object> output = new HashMap<>();
   output.put("id", message.getId());
   // etc
   return output;
}

static Message deserializeMessage(final HashMap<String, Object> map) {
   return new Message(
      (String) map.get("id")),
      helperMethodToSafelyConvertNullableKeyToList(map.get("paragraphs")),
       // etc
   );
}

iOS will need the equivalent implemented in Objective C as well.

Then Dart also needs the equivalent boilerplate written.

class Message {
   Message({@required String id, @required String title //etc
   final String id;
   final String title;
   // etc
}

Map serializeMessage(Message message) {
   return <String, dynamic> {
     id: message.id,
     title: message.title,
     // etc
   };
}

Message deserializeMessage(Map map) {
   return Message(
     id: map["id"],
     title: map["title"],
     // etc
   );
}

In addition to this, all platforms need to have the MethodChannel calls wired together with exactly the same strings to make sure they're being sent and received correctly.

There are tools out there like protocol buffers aimed at solving this problem, and it may be better to bring one in instead of continuing our current pattern. However there would be costs associated with the extra dependency, and we'd most likely need to add a compile step of some kind to the code that used it.

Opening this to track any discussions and investigations around changing how we handle this.

@mklim mklim added c: new feature Nothing broken; request for a new capability team Infra upgrades, team productivity, code health, technical debt. See also team: labels. plugin labels Feb 22, 2019
@mklim mklim added this to the Stretch Goals milestone Feb 22, 2019
@mklim mklim changed the title Improve how transfer structured data over MethodChannels Improve how structured data is transferred over the MethodChannels Feb 22, 2019
@mklim mklim changed the title Improve how structured data is transferred over the MethodChannels Improve structured data handling over the MethodChannels Feb 22, 2019
@zoechi
Copy link
Contributor

zoechi commented Feb 22, 2019

Similar to #18850 where flatbuffers were suggested in addition to protobuf

@amirh
Copy link
Contributor

amirh commented Feb 22, 2019

This also applies to system channels used by the framework.

@dnfield
Copy link
Contributor

dnfield commented Feb 22, 2019

I would advocate moving to a flat or protobuf style message, or perhaps a (subset of) FIDL.

With that, we should be able to support codegen based implementations for the platform side, so that developers wouldn't be stuck writing awful boilerplate. And we could have easy support for basic data structures (maps and lists) and primitives (strings/character data, bytes, ints, etc.).

For reference, react native handles this this way: https://facebook.github.io/react-native/docs/native-modules-ios

@iskakaushik
Copy link
Contributor

Whatever we end up using should have Dart, obj-c, swift, java, kotlin, maybe even c++ bindings. I think this rules out FIDL.

@dnfield
Copy link
Contributor

dnfield commented Feb 22, 2019

We could write them if FIDL is the right fit for other reasons - Flatbuffers didn't support Dart up until recently :)

@jonahwilliams
Copy link
Member

FIDL is the Fuchsia IDL, doesn't make any sense to try and use it for Android/iOS

@mklim
Copy link
Contributor Author

mklim commented Feb 28, 2019

/cc @matthew-carroll. The issue was originally opened with plugins in mind but this general concern also applies to the channels in the engine.

@mklim mklim added the engine flutter/engine repository. See also e: labels. label Feb 28, 2019
@matthew-carroll
Copy link
Contributor

As we consider formalizing aspects of system channels, I'd like to point out a few things:

  • System channels are public APIs even if they're not currently public. Flutter expects these channels to exist and to operate in a particular way, therefore developers are beholden to them no matter what.
  • Our current breakdown of system channels is unfortunate. Some names are rather ambiguous and a number of comms could reasonably be moved to other channels and still make sense. It would be nice to look more critically at the process behind dividing channels, naming channels, etc.
  • In my opinion, any technology that is used to codify system channels should be frictionless when used by external devs. Even if we don't expect external devs to touch system channels very often, the nature of these channels is so important that we should do whatever we can to avoid requiring non-trivial project technologies to work with them. For example, I think protobufs tend to be easy to use internally, but are much more tedious to setup externally.

@matthew-carroll
Copy link
Contributor

Also, I think the very term "system channels" should be re-evaluated. Docs refer to "platform channels" as the official framework channels. However, there is a specific channel called THE "platform channel." The term "system channels" is used within the code. However, there is a specific channel called THE "system channel." The terminology is confusing at best.

@dnfield
Copy link
Contributor

dnfield commented Feb 28, 2019

@matthew-carroll I think System Channels usually refers to the channels the Framework uses to communicate with the engine - this issue is more generally about message channels.

@matthew-carroll
Copy link
Contributor

Fair enough. But I assume in general answer would need to equally address the framework/engine channels, right?

Also, I think my 3rd point probably stands, regardless.

@mklim
Copy link
Contributor Author

mklim commented Feb 28, 2019

I don't think we necessarily need one solution that solves all cases. It's a similar problem across them all, but I think they have different needs from a solution. I could see potentially recommending a practice just for very limited particularly painful plugin use cases in the end because of friction or dependency cost making it too prohibitive to be worth adopting generally. Ideally I think we would find something low friction that would be easy to use though, it may not be worth adopting in any case otherwise.

@mklim
Copy link
Contributor Author

mklim commented Mar 7, 2019

There was a bug caused by this recently in the engine that wasn't detected until the auto roller merged the change into the framework and the flutter/flutter build broke.

flutter/engine#7494 edited blink::PointerData::DeviceKind, a C++ struct that's expected to be perfectly aligned with a dart enum PointerDeviceKind. The two ended up somewhat mismatched in the patch and the engine tests didn't catch it. The issue was fixed after reverting the roll into the framework and setting the types to match again in flutter/engine#8065 and flutter/engine#8066.

@cbracken
Copy link
Member

cbracken commented Mar 7, 2019

Other recent fixes due to mismatches in parallel APIs within the engine include flutter/engine#8033 and flutter/engine#8030.

FWIW These aren't necessarily applicable to the specific problem of plugins, but they have some analogues. I expect the solution in the engine will likely be a combination of things:

  1. Codegen the dart:ui enum, the internal engine enum, and the enum that we expose via the embedder API. Whether the latter two are in fact a single type shared between the embedder API and the engine internals is an open question. The obvious candidate here is a gn build step to do this.
  2. Other embedders will eventually migrate to the embedder API, obviating the need for us (and others) to codegen up their own enums.

@mklim
Copy link
Contributor Author

mklim commented Mar 7, 2019

Discussed more offline with @cbracken and @jonahwilliams: the compile step problem would actually be negligible for the framework/engine part of this. Practically the structs would all live in the engine, which already has a build step. We could most likely work whatever extra codegen we needed into our existing gn scripts.

Unfortunately the concern does still apply to the plugins though.

@matthew-carroll
Copy link
Contributor

@mklim any idea what the developer experience for this proposal would look like? Imagine I'm starting from scratch with a new plugin project. What dependencies do I need and where do I add them? Do I have to modify pubspec or something else to affect change in the build process? Where would I place my declarations of messages? etc.

For me it would help to see the expected/desired process from start to finish to help determine tradeoffs and issues that we need to consider.

@collinjackson
Copy link
Contributor

Note that there are places in firebase_database and cloud_firestore where it's convenient to be able to pass a deeply nested NSDictionary into Dart and MethodChannel turns into a Map automatically. This is definitely the exception rather than the rule though -- most of the time typed data is what you want, and we could find a workaround for the small number of places where dynamic data structures need to be passed across a channel.

@mklim mklim modified the milestones: Stretch Goals, Goals Jul 29, 2019
@mklim mklim self-assigned this Jul 29, 2019
@mklim
Copy link
Contributor Author

mklim commented Oct 1, 2019

Progress update:

I have an unfortunately private RFC that predates our OSS template going into my research into this in detail. There's a brief summary of it in this comment.

One thing worth re-stating here is that there's two problems here: generating the schema API and non-primitive data types. They're related but orthogonal.

I prototyped a rough solution for the IPC/schema portion of this problem a few weeks ago. The rough working prototype I have is here. The API in that prototype needs work, I was just focused on getting something up and running at the time.

Another thing worth saying is that if all somebody wants is to generate the complex data types today, this is technically possible with protocol buffers as they are. But that does require ramp up on the protobuf IDL first, and the protocol compile command would also exist outside of the dart builder workflow. Once protobufs are incorporated into the project, the dev just needs to parse the messages to and from byte buffers on each end of the method channel for the different languages and it will work for complex data types, as-is. I expect the developer experience here would be clunky since it's so much outside of the regular Flutter workflow, so I wouldn't recommend this right now unless it's really worth the extra overhead to avoid duplicating the data structures.

I looked at a few other IDLs, but only protobufs supported all of our languages. That said it's not ideal since it optimizes for size over the wire instead of performance at runtime (for our use case it's performance at runtime that matters, not compressed size).

Unfortunately the milestone has slipped. I'm bumping it out to a more realistic date.

@mklim mklim modified the milestones: September 2019, January 2020 Oct 1, 2019
@mklim mklim added the a: existing-apps Integration with existing apps via the add-to-app flow label Oct 14, 2019
@arctouch-matheusromao
Copy link

Hey guys! Any news on this regard? I'm using protobuf, but proto3 doesn't support constant values :/ (see here why). I really would like to standardize the error messages coming from all the platforms without having to define them in each language.

@t-artikov
Copy link

@mklim
Have you considered Dart itself as a language for messages definition?
You mark messages with annotation and boilerplate code (serialization, foreign language class declarations) is generated automatically (using source_gen).

// MyPlugin.dart
@Message(
    languages: [Language.java, Language.objC]
)
class MyMessage {
  MyMessage({this.id, this.title, this.data});

  final int id;
  final String title;
  final List<String> data;
}

This approach can provide a better workflow than existing solutions(protobuf, flatbuffers) and does not require external dependencies.

@gaaclarke gaaclarke moved this from Awaiting triage to Engineer reviewed in Add-to-app - plugin support review Jan 16, 2020
@mklim
Copy link
Contributor Author

mklim commented Jan 28, 2020

@t-artikov yeah, that's the general approach I took for my prototype. :) I think that's a totally viable approach but there was some concerns that developers would find it confusing to edit IDL definitions within their application Dart source code. I wrote up that approach briefly as a subset of the current Dartle doc and in more detail in its own doc.

@gaaclarke is driving this forward from here, I'm going to reassign this to him and reset to the goals for him to scope.

@mklim mklim assigned gaaclarke and unassigned mklim Jan 28, 2020
@mklim mklim modified the milestones: January 2020, Goals Jan 28, 2020
@t-artikov
Copy link

@mklim
Thanks for sharing!

It’s not clear to me why Dartle is better than Flutterbuffer.
The main benefit of using Dart instead of third-party IDL is the lack of the need to convert IDL structs to Dart ones. As result:

  • You can use structs in the Dart business logic before running code generation;
  • You control the structs code - can add extra methods to use it in Dart:
@Flutterbuffer()
class Contact {
  String name;
  String email;
  int phone;
  List<String> address;

  @override
  String toString() => ... ;
  bool get isValid => ...;
}

I don’t think it’s a problem to be able to mark a non-serializable struct by @Flutterbuffer - codegen tool will report an error.

I understand that there are technical problems in the implementation of Flutterbuffer (ability of writing to arbitrary directories by Dart codegen), but shouldn't it just be solved by the appropriate team?

@jmagman jmagman added this to Awaiting triage in Mobile - Add-to-app review via automation Mar 10, 2020
@jmagman jmagman moved this from Awaiting triage to Engineer reviewed in Mobile - Add-to-app review Apr 30, 2020
@kf6gpe kf6gpe added the P2 Important issues not at the top of the work list label May 29, 2020
@xster
Copy link
Member

xster commented Jun 11, 2020

I'm gonna merge this into #32930 since this is close to done. A sample usage is at flutter/samples#465.

@xster xster closed this as completed Jun 11, 2020
@github-actions
Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 14, 2021
@flutter-triage-bot flutter-triage-bot bot added the package flutter/packages repository. See also p: labels. label Jul 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
a: existing-apps Integration with existing apps via the add-to-app flow c: new feature Nothing broken; request for a new capability engine flutter/engine repository. See also e: labels. P2 Important issues not at the top of the work list package flutter/packages repository. See also p: labels. team Infra upgrades, team productivity, code health, technical debt. See also team: labels.
Projects
Mobile - Add-to-app review
  
Engineer reviewed
Development

No branches or pull requests