[pigeon] Migrates off old BinaryMessenger API #3600
Conversation
github-actions
bot
added
platform-android
platform-ios
platform-macos
platform-windows
labels
| final String messageHandlerSetter = | ||
| isMockHandler ? 'setMockMessageHandler' : 'setMessageHandler'; | ||
| final String messageHandlerSetter = isMockHandler | ||
| ? '_testBinaryMessengerBinding!.defaultBinaryMessenger.setMockDecodedMessageHandler<Object?>(channel, ' |
There was a problem hiding this comment.
is the ! here the _ambiguate equivalent?
There was a problem hiding this comment.
Yes, that plus typing the return type of the getter above as TestDefaultBinaryMessengerBinding? to hide the fact that TestDefaultBinaryMessengerBinding.instance may be non-nullable in certain versions of Flutter.
| indent.addScoped('{', '}', () { | ||
| indent.write( | ||
| 'channel.$messageHandlerSetter((Object? message) async ', | ||
| '$messageHandlerSetter(Object? message) async ', |
There was a problem hiding this comment.
might be worth a comment somewhere calling out that messageHandlerSetter contains the (
There was a problem hiding this comment.
Renamed the variable name to make that clear.
| } | ||
|
|
||
| task clean(type: Delete) { | ||
| tasks.register("clean", Delete) { |
There was a problem hiding this comment.
🤷 This is the diff the generator script produced when re-generating all the generated files.
There was a problem hiding this comment.
This looks like it was probably a flutter migrator from running on master. It's fine if you want to leave it, but it's unrelated to anything in this PR.
|
Was @stuartmorgan in the loop with the BinaryMessenger API change? I just wanted to make sure his team had an opportunity to consider the ramifications. I guess it means loading on old plugins won't be able to run their tests, right? |
| /// | ||
| /// This comment also tests multiple line comments. | ||
| abstract class TestHostApi { | ||
| static TestDefaultBinaryMessengerBinding? get _testBinaryMessengerBinding => |
There was a problem hiding this comment.
Why is this being written once per class instead of at the file level?
There was a problem hiding this comment.
Main motivation was to keep the generator script more readable by generating the _testBinaryMessengerBinding getter close to where it is used and to make sure that the public DartGenerator.writeFlutterApi always generates functioning code - no matter from what context it is called. If we want to generate this only once per file, we'd have to move the getter generation out of DartGenerator.writeFlutterApi (where it will later be used) into DartGenerator.generateTest - it's possible, but overall seems more error-prone and the generator script becomes harder to follow. However, if you prefer doing it only once per file I am happy to change this. Please let me know.
There was a problem hiding this comment.
I'm not as familiar with the test generator; that seems reasonable, so feel free to leave it.
(Long term we want to do more to have shared utilities created in a clear, structured way, but that work may not have even started for this generator.)
| } | ||
|
|
||
| task clean(type: Delete) { | ||
| tasks.register("clean", Delete) { |
There was a problem hiding this comment.
This looks like it was probably a flutter migrator from running on master. It's fine if you want to leave it, but it's unrelated to anything in this PR.
I wasn't but it seems like the ramifications are minimal. Deprecation isn't removal, and a two-year grace period since the new API was introduced is quite generous. I doubt many plugins being actively maintained still actually work on Flutter 2.0. And this case it wouldn't even affect clients on 2.0. |
No, we try as much as possible follow the spirit of semver for Pigeon. Internal details of generated code are like internal details of a package, so can be versioned accordingly. |
goderbauer
added
the
autosubmit
[pigeon] Migrates off old BinaryMessenger API
4 participants
New API has been around for 2-ish years, was introduced in flutter/flutter@5e0cc4c. Old API is to be deprecated in flutter/flutter#116793.
This is changing the code that pigeon generates (see changes indart_generator.dart). Wondering if that requires special handling (e.g. a minor or major version bump).