iOS Devtools is fully functional with Flipper#5
Conversation
2e0e62b to
891d990
Compare
cf8f75b to
5178012
Compare
5178012 to
c9cccb3
Compare
devtools/plugin/core/src/reducer.ts
Outdated
| draft.messages.push(message); | ||
| }); | ||
| case "PLAYER_DEVTOOLS_PLUGIN_INTERACTION": | ||
| console.log("[REDUCER] PLAYER_DEVTOOLS_PLUGIN_INTERACTION received:", transaction); |
There was a problem hiding this comment.
Note to self: remove in next commit
| player.logger.error(this.name, "Error setting the following log: ", this.logs); | ||
| // Silently fail |
There was a problem hiding this comment.
Note to self: undo in next commit
| this.logger?.deref()?.error(this.name, "Error parsing new flow", e); | ||
| console.error("Error parsing new flow", e); |
There was a problem hiding this comment.
Note to self: change back
| public func removeListener(_ listener: @escaping MessageListener) { | ||
| print("DEBUG [iOS DevtoolsFlipperPlugin]: removeListener() called") | ||
| /* TODO: we can't compare listeners directly on ios. | ||
| Implement workaround. E.g. register listeners by ID? */ | ||
| } |
There was a problem hiding this comment.
I would like to change the add/remove listener to work with subscribe/unsubscribe pattern instead, where adding the listener returns the way to remove it, like we discussed offline. E.g.
let unsubscribe = addListener(listener)I think that would work across all the platforms, though I'm hesitant to touch the other platform code for that in this PR.
Is removing the listener need for the MVP? If so, I can change things in this PR. If not, I'd like to break out the change into a separate ticket where I can focus on that one change across all platforms.
There was a problem hiding this comment.
Is removing the listener need for the MVP?
Kinda depends on what happens — if it doesn't break anything, then it's probably not needed. It doesn't look like we have a good way to invoke this anyways, as long as we aren't invoking stale listeners and crashing the app.
I think that would work across all the platforms, though I'm hesitant to touch the other platform code for that in this PR.
That said, this is just the Flipper plugin connection for iOS — as there isn't platform agnostic code here, you should be able to change the API to match what iOS needs w/o having to worry about other platforms. Android would be applicable from a consistency standpoint, but would still be in parity either way.
| let options = MessengerOptions( | ||
| id: playerID, | ||
| jsContext: jsContext, | ||
| context: .player, | ||
| logger: PlayerLogger(logger: player.logger), | ||
| sendMessage: flipperPlugin.sendMessage(_:), | ||
| addListener: flipperPlugin.addListener(_:), | ||
| removeListener: flipperPlugin.removeListener(_:), | ||
| messageCallback: try store.dispatch(event:) | ||
| ) | ||
| let messenger = try Messenger(options: options) | ||
| self.messenger = messenger | ||
| _ = registerMessenger(messenger: messenger) |
There was a problem hiding this comment.
This should come from the base implementation to remove the overhead from folks creating their own devtools plugins.
There was a problem hiding this comment.
Done. Was having a hard time doing this initially but figured it out. 👍🏽
| // player.hooks?.state.tap { state in | ||
| // // TODO: what is happening here on android? | ||
| // let temp: BaseFlowState = .init(status: .completed) | ||
| // } |
There was a problem hiding this comment.
Here we're hooking into teardown logic to deregister Player instance from the FlipperClient. We should also be sending a disconnect event to the actual client to let them know the Player instance is gone.
There was a problem hiding this comment.
I'll see if I can figure out how to do something similar on iOS. 🤔
It's my understanding so far that ReleasedState on android means "the player is going away and being garbage collected" (docs that I'm referencing). iOS doesn't have a counterpart for that, afaict. We only have notStarted, inProgress, completed, and error (link). Are you aware of any ways ios has matched this android pattern in the past? I'm currently exploring deinit.
There was a problem hiding this comment.
Yikes, apparently deinits in Swift aren't actually called when the app terminates. That's fun for me 😆 .
There was a problem hiding this comment.
For the React implementation, they hook into component unmount:
devtools/devtools/plugin/react/src/plugin.tsx
Lines 51 to 54 in 52cd2b0
On Android, we're effectively hooking into viewmodel teardown via released state. I'm not sure you'd have a hook exposed unless one existed for iOS, but there should be a way to hook into unmount I'd expect.
| print("[MessengerOptions] Converting to JSValue") | ||
| print("[MessengerOptions] context:", context) | ||
| print("[MessengerOptions] context.rawValue:", context.rawValue) |
| /// Errors that can occur when trying to load a JS class that will be referenced by a Swift wrapper | ||
| enum JSBaseError: Error { | ||
| case noSuchFile | ||
| case failedToParseScript | ||
| case failedToMakeContext | ||
| case noSuchJSModule | ||
| case noSuchJSClass | ||
| case couldNotInstantiateClass | ||
| } |
There was a problem hiding this comment.
Is this enum something we should be supplying from core Player (or JS loading) lib to encapsulate possible JS errors?
There was a problem hiding this comment.
I would like to supply it from a JS loading library that lives in the player-ui repo. I think these would also be helpful errors for us to propagate when loading JS files for JSBasePlugins, since errors are pretty opaque when something goes wrong.
I put it here for now to limit the number of repos I'd have to touch for the MVP of devtools. I'll make an issue? to move these and drop the link here.
.bazelrc
Outdated
| # common --ios_simulator_device="iPhone 16 Pro" | ||
| # common --ios_simulator_version="18.5" |
There was a problem hiding this comment.
They shouldn't be commented out, no. If you mean the device/version flags, then yes, we do need them. They allow us to ensure a consistent test environment in CI/CD.
There was a problem hiding this comment.
If they're just for CI, should we put them in the CI-specific Bazel config?
FWIW — I did need to comment them out to run the demo app locally.
There was a problem hiding this comment.
Ah, yeah, they could affect your local if you don't run through Xcode and instead do bazel run. It will try to find the matching sim locally and if you don't have one, it'll be unhappy. Although I prefer that to having it pick the first simulator available on your machine, which might be an iPhone 4 running iOS 9 😆 . (I believe that's what Bazel does without something specified.)
I'll move it to the CI-specific place for now though. I think we can deal with the "my infinitely old simulator is not working" if it crops up.
1e4db67 to
ca87efe
Compare
What Changed
Get the iOS Devtools to open and connect to flipper, delivering expected logs and evaluating expressions.
Also:
Why
We want mobile Devtools to make life easier for users of player.
Definition of Done