Repository navigation
Migrate FlutterCodecs and its unit tests to Swift - #190329
LongCatIsLooong wants to merge 11 commits into
Conversation
github-actions
Bot
added
platform-ios
e5d75ab to
0408a61
Compare
| if message is [Any] || message is [AnyHashable: Any] { | ||
| encoding = try? JSONSerialization.data(withJSONObject: message, options: []) | ||
| } else { | ||
| // NSJSONSerialization does not support top-level simple values. |
There was a problem hiding this comment.
Don't think this is true but I'll fix this in a separate PR.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request migrates the FlutterCodecs implementation and its corresponding unit tests from Objective-C to Swift. It also introduces an unexported symbols list to prevent the newly generated Swift symbols from being exported in the Flutter framework's dynamic symbol table. Review feedback suggests using more idiomatic Swift type checks for optional data and restoring several assertions from the original Objective-C implementation to prevent silent failures during decoding.
|
I am wondering a bit what is the benefit of translating random bits of embedder to Swift. The downside seem pretty clear, unfortunately, reducing consistency and increasing friction when navigating between the objc / swift parts of the codebase. |
I think the fraction is only temporary - it should be gone when we migrate most of the old objc codebase to swift (since with @objc @implementation we can just swap out objc implementations with swift ones in theory we should be able to migrate most objc code and keep the public headers in objc). As for benefits, for one Swift's type system is much more capable so it's easier to spot problems during implementation and code review. I filed #192122 when doing the migration since the compiler was able to identify a couple of nullability issues in the objc logic. |
| _$sSo22FlutterJSONMethodCodecCMa | ||
| _$sSo23FlutterJSONMessageCodecC08InternalA11SwiftCommonEABycfC | ||
| _$sSo23FlutterJSONMessageCodecC08InternalA11SwiftCommonEABycfc | ||
| _$sSo23FlutterJSONMessageCodecCMa |
There was a problem hiding this comment.
How are these generated? Do we have to touch this file whenever we update swift api?
| import Flutter | ||
|
|
||
| // This file is to ensure that Flutter.framework can be cleanly imported | ||
| // by Swift clients, and certain Symbols are visible to Swift clients. |
There was a problem hiding this comment.
do we also need to check objc still works?
hellohuanlin
left a comment
hellohuanlin
left a comment
There was a problem hiding this comment.
unexported_symbols.sym file seems like a headache when writing swift code
4 participants
This is an experiment that implements Objective-C APIs using
@objc @implementation extensions (https://github.com/swiftlang/swift-evolution/blob/main/proposals/0436-objc-implementation.md). The migration is mostly done by gemini, told it to translate the code verbatim. Will try to parameterize the tests and fix the JSON top level element handling later.Swift symbols
@objc @implementationextensions introduce visible Swift symbols even though they're just implementing Objective-C APIs. These symbols are stripped by the linker in the:flutter_framework_dylibtarget using a deny list. I've manually verified the producedFlutter.frameworkstill work, even when the client is using a different version of swift compiler.Tests
added
engine/src/flutter/shell/platform/darwin/ios/framework/Source/FlutterUmbrellaImport.swiftfor iOS andengine/src/flutter/shell/platform/darwin/macos/framework/Source/FlutterUmbrellaImportTests.swiftfor macOS to make sure Swift clients can see those symbols.Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.