Added error to callback #2439

fewieden · 2021-01-28T06:48:40Z

Does the pull request solve a related issue?

After adding lockStrings in a merge request of a module we were running into issues because the callback wasn't triggered in case lock strings were active. Which means you only get notified on success. The way nodejs and any popular libraries that still uses callbacks implement them are like callback(error) or callback(error, data). This way the caller gets always notified and can handle based on the error argument.

@skuethe with this, we can avoid manually checking for active lock strings again.

If so, can you reference the issue?

fewieden/MMM-Modal#7 (comment)

What does the pull request accomplish?

callback for show function always gets called.
If lockStrings are still active the callback contains an error

codecov-io · 2021-01-28T07:02:19Z

Codecov Report

Merging #2439 (652e1a5) into develop (ef2fd16) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop    #2439   +/-   ##
========================================
  Coverage    39.33%   39.33%           
========================================
  Files           20       20           
  Lines         1612     1612           
========================================
  Hits           634      634           
  Misses         978      978

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 ef2fd16...652e1a5. Read the comment docs.

rejas · 2021-01-28T07:33:45Z

js/main.js

@@ -295,6 +295,7 @@ var MM = (function () {
 		// Otherwise cancel show action.
 		if (module.lockStrings.length !== 0 && options.force !== true) {
 			Log.log("Will not show " + module.name + ". LockStrings active: " + module.lockStrings.join(","));
+			callback("Active lock strings");


Maybe rename it to "ERR_ACTIVE_LOCK_STRINGS" so it is a little clearer that it is a error string?

Er even better: allow for an optional error callback?

I replaced the error for the callback to an actual error object. If you log the error it gives you an actual stack trace from its origin which helps to debug.

I think adding a second callback just for the error isn't necessary. Also, @rejas just fixed the callback recently. Until now it was never triggered, so we are not breaking existing modules with it.

The problem of this is older modules might not check if the callback contains an error. They might think the action was executed successfully because the callback was called. So this is a somewhat breaking change.

@MichMich if you want me to change it I can, but as I said, @rejas just fixed the callback which means until now the callback was never executed at all https://github.com/MichMich/MagicMirror/pull/2385/files#diff-50260f11109f55b944da641c51de244dffd718cbd77f7e5ce7b9124ab18fe265R437

Ah, true. But still I think a separate error callback is a bit cleaner (and easier to use as a module developer). Or do you disagree?

# Conflicts: # CHANGELOG.md

rejas · 2021-02-06T20:38:29Z

js/main.js

 	 */
-	var showModule = function (module, speed, callback, options) {
+	var showModule = function (module, speed, callback, errorCallback, options) {


woudln't just "error" be the most common name for such a function (here and in the other occurences too)?

not sure error sounds more like an object instead of a function

yes, you are probably right :-)

MichMich · 2021-02-06T21:41:05Z

Just a thought: What if we make the error callback part of the options object?

rejas · 2021-02-07T08:16:24Z

Just a thought: What if we make the error callback part of the options object?

It's a fine balance.... If we do it, why not also make the callback part of it? Not sure if this way or the current one one is more readable or less error prone...

MichMich · 2021-02-07T08:22:05Z

Because that would be a change of API. The regular callback is also something you often use. Error is something you only need in very specific occasions so can be tucked away. IMHO it's good to limit the amount of arguments a method has. Since error will only be interesting for a hand full of devs, it's ok to be tucked away.

Of course we could also allow the main callback to be put in the options object and check to see if the 2nd argument is an object or method. If it is an object the callback can be extracted.

rejas · 2021-02-07T08:37:58Z

You had me at "change of API" :-) I agree, changing API would be some "v3" change that we dont need to put here. +1 for your putting the error callback in the options and have the normal callback for API reasons in the main function call

MichMich · 2021-02-07T09:45:18Z

And are we going to call it errorCallback or onError?

rejas · 2021-02-07T14:36:19Z

I'd go for onError since there are already occurences of that name in the code.

MichMich · 2021-02-07T17:41:55Z

Agree! onError sounds best. :)

# Conflicts: # CHANGELOG.md

fewieden · 2021-02-13T07:45:27Z

Moved errorCallback into options
Renamed to onError
Created PR in the documentation project
Resolved merge conflicts

fewieden added 2 commits January 28, 2021 07:33

add error to module show callback

2324579

updated changelog

afbdacf

rejas reviewed Jan 28, 2021

View reviewed changes

fewieden added 2 commits January 28, 2021 21:13

more error like message

d2a7a3b

use error object for callback to include stack trace

41da6f4

rejas approved these changes Jan 29, 2021

View reviewed changes

fewieden added 3 commits February 6, 2021 11:29

Merge branch 'develop' into feature/add-error-to-callback

84995c9

# Conflicts: # CHANGELOG.md

add error separate callback

88ed5ed

updated changelog

1ed721f

rejas reviewed Feb 6, 2021

View reviewed changes

fewieden mentioned this pull request Feb 6, 2021

Bugfix/update dom on modal close fewieden/MMM-Modal#7

Merged

rejas approved these changes Feb 6, 2021

View reviewed changes

move error callback into options and rename it

9d85bae

fewieden mentioned this pull request Feb 13, 2021

Add onError callback in show MagicMirrorOrg/MagicMirror-Documentation#49

Merged

Merge branch 'develop' into feature/add-error-to-callback

652e1a5

# Conflicts: # CHANGELOG.md

rejas approved these changes Feb 15, 2021

View reviewed changes

MichMich added 2 commits February 23, 2021 14:10

Merge branch 'develop' into feature/add-error-to-callback

13010ec

Change error string.

b2a21b9

MichMich merged commit ea6eebd into MagicMirrorOrg:develop Feb 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added error to callback #2439

Added error to callback #2439

fewieden commented Jan 28, 2021

codecov-io commented Jan 28, 2021 •

edited

Loading

rejas Jan 28, 2021

MichMich Jan 28, 2021

fewieden Jan 28, 2021 •

edited

Loading

MichMich Jan 30, 2021

fewieden Jan 31, 2021

MichMich Jan 31, 2021 •

edited

Loading

rejas Feb 6, 2021

fewieden Feb 6, 2021

rejas Feb 6, 2021

MichMich commented Feb 6, 2021

rejas commented Feb 7, 2021

MichMich commented Feb 7, 2021

rejas commented Feb 7, 2021

MichMich commented Feb 7, 2021

rejas commented Feb 7, 2021

MichMich commented Feb 7, 2021

fewieden commented Feb 13, 2021

Added error to callback #2439

Added error to callback #2439

Conversation

fewieden commented Jan 28, 2021

codecov-io commented Jan 28, 2021 • edited Loading

Codecov Report

rejas Jan 28, 2021

Choose a reason for hiding this comment

MichMich Jan 28, 2021

Choose a reason for hiding this comment

fewieden Jan 28, 2021 • edited Loading

Choose a reason for hiding this comment

MichMich Jan 30, 2021

Choose a reason for hiding this comment

fewieden Jan 31, 2021

Choose a reason for hiding this comment

MichMich Jan 31, 2021 • edited Loading

Choose a reason for hiding this comment

rejas Feb 6, 2021

Choose a reason for hiding this comment

fewieden Feb 6, 2021

Choose a reason for hiding this comment

rejas Feb 6, 2021

Choose a reason for hiding this comment

MichMich commented Feb 6, 2021

rejas commented Feb 7, 2021

MichMich commented Feb 7, 2021

rejas commented Feb 7, 2021

MichMich commented Feb 7, 2021

rejas commented Feb 7, 2021

MichMich commented Feb 7, 2021

fewieden commented Feb 13, 2021

codecov-io commented Jan 28, 2021 •

edited

Loading

fewieden Jan 28, 2021 •

edited

Loading

MichMich Jan 31, 2021 •

edited

Loading