refactor(client): create Typescript MethodChannel on all platforms #2298
Conversation
client/src/www/app/main.cordova.ts
Outdated
| // TODO(fortuna): wire generic calls in the Cordova plugin. | ||
| return pluginExecWithErrorCode<string>('fetchResource', params); | ||
| default: | ||
| return await pluginExecWithErrorCode( |
There was a problem hiding this comment.
[NOT THIS PR] I feel the name pluginExecWithErrorCode can be updated, instead of returning error codes, it throws a PlatformError now.
client/src/www/app/main.electron.ts
Outdated
| async invokeMethod(methodName: string, params: string): Promise<string> { | ||
| switch (methodName) { | ||
| default: | ||
| return await window.electron.methodChannel.invoke( |
There was a problem hiding this comment.
Maybe the await (and the async above) is not needed here? IIRC, methodChannel.invoke should already return a Promise<string>.
There was a problem hiding this comment.
I think it's clearer if we call await. Otherwise it reads as if we are returning a promise of a promise, even though Typescript auto-unwraps it.
There was a problem hiding this comment.
I would not recommend return invoke(...) in an async function. Instead, I would maybe prefer return invoke(...) in a regular function, since we are not doing any multiple asynchronous steps here.
There was a problem hiding this comment.
Done. But I also removed the switch to avoid mistakes. Previously I was implementing an interception there, and I needed the await.
client/src/www/app/main.electron.ts
Outdated
| async invokeMethod(methodName: string, params: string): Promise<string> { | ||
| switch (methodName) { | ||
| default: | ||
| return await window.electron.methodChannel.invoke( |
There was a problem hiding this comment.
I think it's clearer if we call await. Otherwise it reads as if we are returning a promise of a promise, even though Typescript auto-unwraps it.
client/src/cordova/plugin/android/java/org/outline/OutlinePlugin.java
Outdated
Show resolved
Hide resolved
| } | ||
| DDLogInfo("Fetching resource from \(url)") | ||
| guard let input = command.argument(at: 1) as? String else { | ||
| return sendError("Missing method input", callbackId: command.callbackId) |
There was a problem hiding this comment.
What if the method doesn't accept any input? Passing "" or null?
There was a problem hiding this comment.
I'd say we should always pass an input, even if it's the empty string.
client/src/www/app/main.electron.ts
Outdated
| async invokeMethod(methodName: string, params: string): Promise<string> { | ||
| switch (methodName) { | ||
| default: | ||
| return await window.electron.methodChannel.invoke( |
There was a problem hiding this comment.
I would not recommend return invoke(...) in an async function. Instead, I would maybe prefer return invoke(...) in a regular function, since we are not doing any multiple asynchronous steps here.
|
Tested on macOS, Android and Linux, so I'm quite confident it works on both Cordova and Electron. |
With this change we can call native methods uniformly across Electron and Cordova, and we can easily add new methods by changing Go code only! No more touching all the platforms!
It also allows us to intercept a method at any layer.
I've replaced the
fetchResourcecordova plugin method with anInvokeMethodcall instead to simplify the plugin and demonstrate the functionality.