Stable error handling for loaded Cordova plugins

[mathiasscheffe]

Platforms affected

All

Motivation and Context

When developing a large Cordova app in a distributed team occasionally a developer commits a Cordova plugin which contains an issue which directly results in a javaScript exception on plugin load.
Before the fix the Cordova startup crashes and the app results in a whitescreen.

This can also happen if the plugin uses ES6+ features and the client installs the app on an e.g. older Android device. Such cases should be found during regression testing but sometimes it is just overlooked. With this fix the client gets a descriptive error message.

Description

The fix makes handlePluginsObject more stable. The function keeps track of the loaded modules. If a load error occurs for a module the module is removed from the modules list. Additionally an alert is displayed. This makes it way easier for developers to track and fix the error.

Testing

The 4 existing unit tests of pluginmanager are very high level. If somebody can outline a test idea for the introduced logic I am happy to implement.

Checklist

• 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)
• I've updated the documentation if necessary

[raphinesse]

Thanks for this PR. 👍

I have left two suggestions to simplify the code. It would be great if you could see if they work for you. Unit tests for this feature would be nice as well.

One other thought: It might be better if we only show one message in the end with all problems aggregated. Opinions?

[mathiasscheffe]

Regarding one consolidated error message I had two thoughts in mind:

1. small form factors: When you are running cordova on a phone and then get e.g. 10 modules with an error the message will most likely be larger than the screen size. This will then bring scrolling and awkward layout.
2. probability: In the scenarios I know from the past year a crash while loading a plugin mostly occurs when you develop plugins yourself and in your last builds a mistake was checked in. It is very unlikely that a huge list of plugins fail loading.

Before the PR is not accepted I will create a consolidated error message but I feel more comfortable with the existing solution.

[codecov-io]

Codecov Report

❗ No coverage uploaded for pull request base (master@17ed8ef). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master     #222   +/-   ##
=========================================
  Coverage          ?   82.88%           
=========================================
  Files             ?       14           
  Lines             ?      555           
  Branches          ?        0           
=========================================
  Hits              ?      460           
  Misses            ?       95           
  Partials          ?        0           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 17ed8ef...d24ee06. Read the comment docs.

[mathiasscheffe]

Unit tests for this feature would be nice as well.

Do you have any suggestions on how to hook into the functionality? This will save me some debugging. The existing unit tests are of higher level.
I can make time in 2 weeks to implement unit tests.

[raphinesse]

@mathiasscheffe Good points about the error messages. I think having separate messages should be fine then.

Do you have any suggestions on how to hook into the functionality? This will save me some debugging. The existing unit tests are of higher level.

I think you should be able to define a fake cordova/plugin_list module via cordova.define (see test.require.js for examples) with some faulty plugin modules, spy on window.alert and console.error and check that they get called when loading plugin modules.

I can make time in 2 weeks to implement unit tests.

No hurry. All in due time.

[raphinesse]

Another important point that should be considered is the following: I see the appeal of alert during development, so you notice your mistake ASAP. But under no circumstances should Cordova show an alert in a released App. Network or file system issues could cause this in the current version of this PR AFAICT. What can we do to prevent this?

[mathiasscheffe]

Another important point that should be considered is the following: I see the appeal of alert during development, so you notice your mistake ASAP. But under no circumstances should Cordova show an alert in a released App. Network or file system issues could cause this in the current version of this PR AFAICT. What can we do to prevent this?

I have changed the approach here. I have removed the alert() and have left a comment.
In the Cordova after_platform_add hook each app can use this comment as a search & replace marker and inline an app specific error handling.
I will change our internal app DEV build to inline an alert. For the appstore release build I will do something which does more suit the end user.
Hope this is ok.

P.S.: Will make time until end of week to also implement unit tests.

[mathiasscheffe]

The pull request now also contains unit tests for the newly added functionality.
If you want to debug the unit tests you need a modified karma.conf.js and the build tools need to be adapted to skip creation of the code instrumentation. I have appended these patched files for development purposes as tar.gz.
developmentFiles.tar.gz