You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cordova.apache.org by GitBox <gi...@apache.org> on 2020/03/30 14:39:05 UTC
[GitHub] [cordova-js] mathiasscheffe opened a new pull request #222: Stable
error handling for loaded Cordova plugins
mathiasscheffe opened a new pull request #222: Stable error handling for loaded Cordova plugins
URL: https://github.com/apache/cordova-js/pull/222
**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.
**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.
<!--
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!
-->
### Platforms affected
### 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. -->
### Description
<!-- Describe your changes in detail -->
### Testing
<!-- Please describe in detail how you tested your changes. -->
### 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](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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org
[GitHub] [cordova-js] raphinesse commented on a change in pull request #222:
Stable error handling for loaded Cordova plugins
Posted by GitBox <gi...@apache.org>.
raphinesse commented on a change in pull request #222: Stable error handling for loaded Cordova plugins
URL: https://github.com/apache/cordova-js/pull/222#discussion_r406054113
##########
File path: src/common/pluginloader.js
##########
@@ -79,19 +79,63 @@ function onScriptLoadingComplete (moduleList, finishPluginLoading) {
function handlePluginsObject (path, moduleList, finishPluginLoading) {
// Now inject the scripts.
var scriptCounter = moduleList.length;
+ var modulesWithProblems = [];
if (!scriptCounter) {
finishPluginLoading();
return;
}
+
+ function callOnScriptLoadingComplete () {
+ modulesWithProblems.forEach(function (elem) {
+ var foundIndex = -1;
+ var modulesLength = moduleList.length;
+ var i = 0;
+ // find module with load problems in module list. Use while loop to exit iteration early.
+ while (i < modulesLength && foundIndex === -1) {
+ if (moduleList[i].id === elem) {
+ foundIndex = i;
+ }
+ i++;
+ }
+ if (foundIndex !== -1) {
+ // delete module with load problem from module list
+ moduleList.splice(foundIndex, 1);
+ }
+ });
+ onScriptLoadingComplete(moduleList, finishPluginLoading);
Review comment:
```suggestion
var loadedModules = moduleList.filter(function (m) {
return modulesWithProblems.indexOf(m.id) === -1;
});
onScriptLoadingComplete(loadedModules, finishPluginLoading);
```
That should get the job done as well while making the intention clearer. Plus we do not modify the original array.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org
[GitHub] [cordova-js] codecov-io commented on issue #222: Stable error
handling for loaded Cordova plugins
Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #222: Stable error handling for loaded Cordova plugins
URL: https://github.com/apache/cordova-js/pull/222#issuecomment-611482332
# [Codecov](https://codecov.io/gh/apache/cordova-js/pull/222?src=pr&el=h1) Report
> :exclamation: No coverage uploaded for pull request base (`master@17ed8ef`). [Click here to learn what that means](https://docs.codecov.io/docs/error-reference#section-missing-base-commit).
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/cordova-js/pull/222?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #222 +/- ##
=========================================
Coverage ? 82.82%
=========================================
Files ? 12
Lines ? 553
Branches ? 0
=========================================
Hits ? 458
Misses ? 95
Partials ? 0
```
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/cordova-js/pull/222?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/cordova-js/pull/222?src=pr&el=footer). Last update [17ed8ef...d24ee06](https://codecov.io/gh/apache/cordova-js/pull/222?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org
[GitHub] [cordova-js] raphinesse commented on a change in pull request #222:
Stable error handling for loaded Cordova plugins
Posted by GitBox <gi...@apache.org>.
raphinesse commented on a change in pull request #222: Stable error handling for loaded Cordova plugins
URL: https://github.com/apache/cordova-js/pull/222#discussion_r406147039
##########
File path: src/common/pluginloader.js
##########
@@ -79,19 +79,63 @@ function onScriptLoadingComplete (moduleList, finishPluginLoading) {
function handlePluginsObject (path, moduleList, finishPluginLoading) {
// Now inject the scripts.
var scriptCounter = moduleList.length;
+ var modulesWithProblems = [];
if (!scriptCounter) {
finishPluginLoading();
return;
}
+
+ function callOnScriptLoadingComplete () {
+ modulesWithProblems.forEach(function (elem) {
+ var foundIndex = -1;
+ var modulesLength = moduleList.length;
+ var i = 0;
+ // find module with load problems in module list. Use while loop to exit iteration early.
+ while (i < modulesLength && foundIndex === -1) {
+ if (moduleList[i].id === elem) {
+ foundIndex = i;
+ }
+ i++;
+ }
+ if (foundIndex !== -1) {
+ // delete module with load problem from module list
+ moduleList.splice(foundIndex, 1);
+ }
+ });
+ onScriptLoadingComplete(moduleList, finishPluginLoading);
Review comment:
I don't expect any noticeable slowdown due to the single iteration over all modules. If that should turn out to be a problem, we can still fast-track the no-error case by doing
```js
var loadedModules = modulesWithProblems.length === 0
? moduleList
: moduleList.filter(function (m) {
return modulesWithProblems.indexOf(m.id) === -1;
});
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org
[GitHub] [cordova-js] codecov-io edited a comment on issue #222: Stable
error handling for loaded Cordova plugins
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #222: Stable error handling for loaded Cordova plugins
URL: https://github.com/apache/cordova-js/pull/222#issuecomment-611482332
# [Codecov](https://codecov.io/gh/apache/cordova-js/pull/222?src=pr&el=h1) Report
> :exclamation: No coverage uploaded for pull request base (`master@17ed8ef`). [Click here to learn what that means](https://docs.codecov.io/docs/error-reference#section-missing-base-commit).
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/cordova-js/pull/222?src=pr&el=tree)
```diff
@@ 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](https://codecov.io/gh/apache/cordova-js/pull/222?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/cordova-js/pull/222?src=pr&el=footer). Last update [17ed8ef...d24ee06](https://codecov.io/gh/apache/cordova-js/pull/222?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org
[GitHub] [cordova-js] mathiasscheffe commented on issue #222: Stable error
handling for loaded Cordova plugins
Posted by GitBox <gi...@apache.org>.
mathiasscheffe commented on issue #222: Stable error handling for loaded Cordova plugins
URL: https://github.com/apache/cordova-js/pull/222#issuecomment-611483070
> 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.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org
[GitHub] [cordova-js] codecov-io edited a comment on issue #222: Stable
error handling for loaded Cordova plugins
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #222: Stable error handling for loaded Cordova plugins
URL: https://github.com/apache/cordova-js/pull/222#issuecomment-611482332
# [Codecov](https://codecov.io/gh/apache/cordova-js/pull/222?src=pr&el=h1) Report
> :exclamation: No coverage uploaded for pull request base (`master@17ed8ef`). [Click here to learn what that means](https://docs.codecov.io/docs/error-reference#section-missing-base-commit).
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/cordova-js/pull/222?src=pr&el=tree)
```diff
@@ 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](https://codecov.io/gh/apache/cordova-js/pull/222?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/cordova-js/pull/222?src=pr&el=footer). Last update [17ed8ef...d24ee06](https://codecov.io/gh/apache/cordova-js/pull/222?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org
[GitHub] [cordova-js] raphinesse commented on a change in pull request #222:
Stable error handling for loaded Cordova plugins
Posted by GitBox <gi...@apache.org>.
raphinesse commented on a change in pull request #222: Stable error handling for loaded Cordova plugins
URL: https://github.com/apache/cordova-js/pull/222#discussion_r406035044
##########
File path: src/common/pluginloader.js
##########
@@ -79,19 +79,63 @@ function onScriptLoadingComplete (moduleList, finishPluginLoading) {
function handlePluginsObject (path, moduleList, finishPluginLoading) {
// Now inject the scripts.
var scriptCounter = moduleList.length;
+ var modulesWithProblems = [];
if (!scriptCounter) {
finishPluginLoading();
return;
}
+
+ function callOnScriptLoadingComplete () {
+ modulesWithProblems.forEach(function (elem) {
+ var foundIndex = -1;
+ var modulesLength = moduleList.length;
+ var i = 0;
+ // find module with load problems in module list. Use while loop to exit iteration early.
+ while (i < modulesLength && foundIndex === -1) {
+ if (moduleList[i].id === elem) {
+ foundIndex = i;
+ }
+ i++;
+ }
+ if (foundIndex !== -1) {
+ // delete module with load problem from module list
+ moduleList.splice(foundIndex, 1);
+ }
+ });
+ onScriptLoadingComplete(moduleList, finishPluginLoading);
+ }
+
function scriptLoadedCallback () {
if (!--scriptCounter) {
- onScriptLoadingComplete(moduleList, finishPluginLoading);
+ callOnScriptLoadingComplete();
+ }
+ }
+
+ function scriptLoadedErrorCallback (id, message, source, lineno, colno, error) {
+ modulesWithProblems.push(id);
+ if (typeof message !== 'undefined') {
+ var messageString = message;
+ if (typeof message !== 'string') {
+ messageString = JSON.stringify(message);
+ }
+ messageString = 'Could not load all functions. Please confirm or restart your application. \n \n' +
+ 'Details: Error while loading module: "' + id + '". Module will be skipped. ' + messageString;
+ console.error(messageString);
+ alert(messageString);
+ }
+ if (!--scriptCounter) {
+ callOnScriptLoadingComplete();
}
}
for (var i = 0; i < moduleList.length; i++) {
- injectIfNecessary(moduleList[i].id, path + moduleList[i].file, scriptLoadedCallback);
+ var moduleId = moduleList[i].id;
+ var me = this;
+ // bound function to have the module id when the error occurs.
+ var boundErrorCallback = scriptLoadedErrorCallback.bind(me, moduleId);
Review comment:
```suggestion
// bound function to have the module id when the error occurs.
var boundErrorCallback = scriptLoadedErrorCallback.bind(null, moduleId);
```
Since `scriptLoadedErrorCallback` does not use `this`, there's no need to bind it.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org
[GitHub] [cordova-js] mathiasscheffe commented on a change in pull request
#222: Stable error handling for loaded Cordova plugins
Posted by GitBox <gi...@apache.org>.
mathiasscheffe commented on a change in pull request #222: Stable error handling for loaded Cordova plugins
URL: https://github.com/apache/cordova-js/pull/222#discussion_r406137281
##########
File path: src/common/pluginloader.js
##########
@@ -79,19 +79,63 @@ function onScriptLoadingComplete (moduleList, finishPluginLoading) {
function handlePluginsObject (path, moduleList, finishPluginLoading) {
// Now inject the scripts.
var scriptCounter = moduleList.length;
+ var modulesWithProblems = [];
if (!scriptCounter) {
finishPluginLoading();
return;
}
+
+ function callOnScriptLoadingComplete () {
+ modulesWithProblems.forEach(function (elem) {
+ var foundIndex = -1;
+ var modulesLength = moduleList.length;
+ var i = 0;
+ // find module with load problems in module list. Use while loop to exit iteration early.
+ while (i < modulesLength && foundIndex === -1) {
+ if (moduleList[i].id === elem) {
+ foundIndex = i;
+ }
+ i++;
+ }
+ if (foundIndex !== -1) {
+ // delete module with load problem from module list
+ moduleList.splice(foundIndex, 1);
+ }
+ });
+ onScriptLoadingComplete(moduleList, finishPluginLoading);
Review comment:
I can also agree to this version. Reasons why I decided to implement differently is that
the version I implemented is faster as it in the outer loop only iterates over the smaller array of modules with problems. If there are no problems then no code is executed. But this does not matter that much as the module list is most likely just containing <100 elements.
I'll wait on your comment to that and I am open to accept the suggestion.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org
[GitHub] [cordova-js] raphinesse commented on issue #222: Stable error
handling for loaded Cordova plugins
Posted by GitBox <gi...@apache.org>.
raphinesse commented on issue #222: Stable error handling for loaded Cordova plugins
URL: https://github.com/apache/cordova-js/pull/222#issuecomment-611489850
@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.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org
[GitHub] [cordova-js] mathiasscheffe commented on a change in pull request
#222: Stable error handling for loaded Cordova plugins
Posted by GitBox <gi...@apache.org>.
mathiasscheffe commented on a change in pull request #222: Stable error handling for loaded Cordova plugins
URL: https://github.com/apache/cordova-js/pull/222#discussion_r406134203
##########
File path: src/common/pluginloader.js
##########
@@ -79,19 +79,63 @@ function onScriptLoadingComplete (moduleList, finishPluginLoading) {
function handlePluginsObject (path, moduleList, finishPluginLoading) {
// Now inject the scripts.
var scriptCounter = moduleList.length;
+ var modulesWithProblems = [];
if (!scriptCounter) {
finishPluginLoading();
return;
}
+
+ function callOnScriptLoadingComplete () {
+ modulesWithProblems.forEach(function (elem) {
+ var foundIndex = -1;
+ var modulesLength = moduleList.length;
+ var i = 0;
+ // find module with load problems in module list. Use while loop to exit iteration early.
+ while (i < modulesLength && foundIndex === -1) {
+ if (moduleList[i].id === elem) {
+ foundIndex = i;
+ }
+ i++;
+ }
+ if (foundIndex !== -1) {
+ // delete module with load problem from module list
+ moduleList.splice(foundIndex, 1);
+ }
+ });
+ onScriptLoadingComplete(moduleList, finishPluginLoading);
+ }
+
function scriptLoadedCallback () {
if (!--scriptCounter) {
- onScriptLoadingComplete(moduleList, finishPluginLoading);
+ callOnScriptLoadingComplete();
+ }
+ }
+
+ function scriptLoadedErrorCallback (id, message, source, lineno, colno, error) {
+ modulesWithProblems.push(id);
+ if (typeof message !== 'undefined') {
+ var messageString = message;
+ if (typeof message !== 'string') {
+ messageString = JSON.stringify(message);
+ }
+ messageString = 'Could not load all functions. Please confirm or restart your application. \n \n' +
+ 'Details: Error while loading module: "' + id + '". Module will be skipped. ' + messageString;
+ console.error(messageString);
+ alert(messageString);
+ }
+ if (!--scriptCounter) {
+ callOnScriptLoadingComplete();
}
}
for (var i = 0; i < moduleList.length; i++) {
- injectIfNecessary(moduleList[i].id, path + moduleList[i].file, scriptLoadedCallback);
+ var moduleId = moduleList[i].id;
+ var me = this;
+ // bound function to have the module id when the error occurs.
+ var boundErrorCallback = scriptLoadedErrorCallback.bind(me, moduleId);
Review comment:
This looks fine for me as well.
My intention was to create as little surprises as possible for future developers. Therefore I wanted to keep the this pointer intact. But it is not needed now and will also work in the way you suggested.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org
[GitHub] [cordova-js] codecov-io edited a comment on issue #222: Stable
error handling for loaded Cordova plugins
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #222: Stable error handling for loaded Cordova plugins
URL: https://github.com/apache/cordova-js/pull/222#issuecomment-611482332
# [Codecov](https://codecov.io/gh/apache/cordova-js/pull/222?src=pr&el=h1) Report
> :exclamation: No coverage uploaded for pull request base (`master@17ed8ef`). [Click here to learn what that means](https://docs.codecov.io/docs/error-reference#section-missing-base-commit).
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/cordova-js/pull/222?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #222 +/- ##
=========================================
Coverage ? 82.82%
=========================================
Files ? 12
Lines ? 553
Branches ? 0
=========================================
Hits ? 458
Misses ? 95
Partials ? 0
```
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/cordova-js/pull/222?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/cordova-js/pull/222?src=pr&el=footer). Last update [17ed8ef...d24ee06](https://codecov.io/gh/apache/cordova-js/pull/222?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org
[GitHub] [cordova-js] mathiasscheffe commented on a change in pull request
#222: Stable error handling for loaded Cordova plugins
Posted by GitBox <gi...@apache.org>.
mathiasscheffe commented on a change in pull request #222: Stable error handling for loaded Cordova plugins
URL: https://github.com/apache/cordova-js/pull/222#discussion_r406137281
##########
File path: src/common/pluginloader.js
##########
@@ -79,19 +79,63 @@ function onScriptLoadingComplete (moduleList, finishPluginLoading) {
function handlePluginsObject (path, moduleList, finishPluginLoading) {
// Now inject the scripts.
var scriptCounter = moduleList.length;
+ var modulesWithProblems = [];
if (!scriptCounter) {
finishPluginLoading();
return;
}
+
+ function callOnScriptLoadingComplete () {
+ modulesWithProblems.forEach(function (elem) {
+ var foundIndex = -1;
+ var modulesLength = moduleList.length;
+ var i = 0;
+ // find module with load problems in module list. Use while loop to exit iteration early.
+ while (i < modulesLength && foundIndex === -1) {
+ if (moduleList[i].id === elem) {
+ foundIndex = i;
+ }
+ i++;
+ }
+ if (foundIndex !== -1) {
+ // delete module with load problem from module list
+ moduleList.splice(foundIndex, 1);
+ }
+ });
+ onScriptLoadingComplete(moduleList, finishPluginLoading);
Review comment:
I can also agree to this version. Reasons why I decided to implement differently is that
the version I implemented is faster as it in the outer loop only iterates over the smaller array of modules with problems. If there are no problems then no code is executed. But this does not matter that much as the module list is most likely just containing <100 elements.
I'll wait on your comment to that and I am open to accept the suggestion.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org
[GitHub] [cordova-js] raphinesse commented on issue #222: Stable error
handling for loaded Cordova plugins
Posted by GitBox <gi...@apache.org>.
raphinesse commented on issue #222: Stable error handling for loaded Cordova plugins
URL: https://github.com/apache/cordova-js/pull/222#issuecomment-611491579
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?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org
[GitHub] [cordova-js] codecov-io edited a comment on issue #222: Stable
error handling for loaded Cordova plugins
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #222: Stable error handling for loaded Cordova plugins
URL: https://github.com/apache/cordova-js/pull/222#issuecomment-611482332
# [Codecov](https://codecov.io/gh/apache/cordova-js/pull/222?src=pr&el=h1) Report
> :exclamation: No coverage uploaded for pull request base (`master@17ed8ef`). [Click here to learn what that means](https://docs.codecov.io/docs/error-reference#section-missing-base-commit).
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/cordova-js/pull/222?src=pr&el=tree)
```diff
@@ 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](https://codecov.io/gh/apache/cordova-js/pull/222?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/cordova-js/pull/222?src=pr&el=footer). Last update [17ed8ef...d24ee06](https://codecov.io/gh/apache/cordova-js/pull/222?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org
[GitHub] [cordova-js] codecov-io edited a comment on issue #222: Stable
error handling for loaded Cordova plugins
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #222: Stable error handling for loaded Cordova plugins
URL: https://github.com/apache/cordova-js/pull/222#issuecomment-611482332
# [Codecov](https://codecov.io/gh/apache/cordova-js/pull/222?src=pr&el=h1) Report
> :exclamation: No coverage uploaded for pull request base (`master@17ed8ef`). [Click here to learn what that means](https://docs.codecov.io/docs/error-reference#section-missing-base-commit).
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/cordova-js/pull/222?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #222 +/- ##
=========================================
Coverage ? 82.82%
=========================================
Files ? 12
Lines ? 553
Branches ? 0
=========================================
Hits ? 458
Misses ? 95
Partials ? 0
```
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/cordova-js/pull/222?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/cordova-js/pull/222?src=pr&el=footer). Last update [17ed8ef...d24ee06](https://codecov.io/gh/apache/cordova-js/pull/222?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org
[GitHub] [cordova-js] mathiasscheffe commented on issue #222: Stable error
handling for loaded Cordova plugins
Posted by GitBox <gi...@apache.org>.
mathiasscheffe commented on issue #222: Stable error handling for loaded Cordova plugins
URL: https://github.com/apache/cordova-js/pull/222#issuecomment-611479356
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.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org