[GitHub] [cordova-electron] fabiofabbri84 opened a new pull request, #239: breaking: improve support for plugins code running in main, fixing quirks of 3.x, and making it more coherent with Cordova APIs

[GitBox <gi...@apache.org>]

fabiofabbri84 opened a new pull request, #239:
URL: https://github.com/apache/cordova-electron/pull/239

   <!--
   Please make sure the checklist boxes are all checked before submitting the PR. The checklist is intended as a quick reference, for complete details please see our Contributor Guidelines:
   
   http://cordova.apache.org/contribute/contribute_guidelines.html
   
   Thanks!
   -->
   
   ### Motivation and Context
   <!-- Why is this change required? What problem does it solve? -->
   <!-- If it fixes an open issue, please link to the issue here. -->
   - In cordova-electron 3.x, the interface between code running in the main process and in the renderer, is based purely on ipcRenderer, and the "success" callback is called when a value is returned, and "error" callback is called if an exception is thrown or a rejected promise is returned.
   - However, using this method, the object thrown or the rejected promise parameter is converted to String and encapsulated in an error, see https://github.com/electron/electron/issues/24427
   - usually, Cordova APIs relies on "success" and "error" callbacks, to which you can pass an argument, so I think it's better to keep this standard
   - A quirk I noticed investigating this, is that the plugin version receives the parameters encapsulated twice in an Array, see also https://github.com/apache/cordova-electron/issues/214 .
   - As the standard used in cordova-electron 3.x is not documented and has some improvable quirks, this might be a good moment to make it more coherent with other Cordova APIs, at the same time allowing plugin developers to support both 3.x and 4.x standards (and even 3.2 if https://github.com/apache/cordova-electron/pull/238 is accepted)
   
   
   ### Description
   <!-- Describe your changes in detail -->
   On the main side:
   - "...args" is replaced with "args" to address https://github.com/apache/cordova-electron/issues/214
   - A promise is returned
   - the plugin action is called with arguments (success, error, args), to make it more coherent with other Cordova APIs and to make it easy to check if we are using  4.x standard (typeof first_argument == 'function') or 3.x standard (otherwise first argument is an array containing an array of arguments)
   - the first time success or error functions are called, the promise is resolved, passing an object with property "success" true or false depending on the function called, and property "result" containing the parameter passed to the function.
   
   On the renderer side:
   - An object is expected to be returned from the main. If it's property "success" is true, the success callback is called passing property result as parameter, otherwise error is called the same way.
   - In case of error, the error value is passed to the error callback, but this should not happen.
   - Before calling "success" or "error" callback, we check if they actually are functions, to avoid annoying error messages if they were not defined.
   
   
   
   ### Testing
   <!-- Please describe in detail how you tested your changes. -->
   I made demo plugin, based on plugman example, that supports this proposal, cordova-electron 3.1, and my proposal for cordova-electron 3.2. You can find it here: https://github.com/cimatti/cordova-electron-v4proposal-demo-plugin
   
   You can create a demo project using this plugin with these commands:
   ```
   cordova create mytest
   cd mytest
   cordova platform add electron@3.1
   cordova plugin add https://github.com/cimatti/cordova-electron-v4proposal-demo-plugin
   cordova run electron --nobuild
   ```
   
   On the electron app console, you can call the following JavaScript commands to test the problems with the current implementation:
   ```
   // Defining the callbacks
   var successCB = function(result){console.log("success");console.log(result)}
   var errorCB = function(e){console.log("error");console.log(e)}
   
   DemoPlugin.coolMethod('',successCB) // should do nothing
   // With cordova-electron 3.x you get an uncaught exception in the log if success or error callback functions are invoked but are omitted from the arguments
   
   DemoPlugin.coolMethod('',successCB, errorCB) // should log 'error' and the error message string
   // With cordova-electron 3.x the error function argument is always converted to string and encapsulated in an Error object
   
   DemoPlugin.coolMethod('hi', successCB, errorCB)
   // should log 'success' and 'hi'
   ```
   Now you can use these commands to replace cordova-electron 3.1 with my 4.0 proposal:
   ```
   cordova platform remove electron
   cordova platform add https://github.com/cimatti/cordova-electron.git#4.0.x-proposal
   cordova run electron --nobuild
   ```
   
   Calling again the JavaScript console functions above, you can see that everything is running as expected
   
   Now you can also test this demo plugin with my cordova-electron 3.2 proposal (see https://github.com/apache/cordova-electron/pull/238 ) to check how is possible to make a plugin compatible with cordova-electon 3.1 and my proposals for cordova-electron 3.2 and 4.0
   ```
   cordova platform remove electron
   cordova platform add https://github.com/cimatti/cordova-electron.git#3.2.x-proposal
   cordova run electron --nobuild
   ```
   
   ### Checklist
   
   - [x] I've run the tests to see all new and existing tests pass
   - [ ] I added automated test coverage as appropriate for this change
   - [ ] Commit is prefixed with `(platform)` if this change only applies to one platform (e.g. `(android)`)
   - [ ] If this Pull Request resolves an issue, I linked to the issue in the text above (and used the correct [keyword to close issues using keywords](https://help.github.com/articles/closing-issues-using-keywords/))
   - [ ] I've updated the documentation if necessary
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org