Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve callback error handling #211

Merged
merged 3 commits into from Oct 25, 2019
Merged

Conversation

Lindsay-Needs-Sleep
Copy link
Contributor

@Lindsay-Needs-Sleep Lindsay-Needs-Sleep commented Sep 23, 2019

Platforms affected

All

Motivation and Context

Problems solved:

  • Allow users to get the stack trace for errors via cordovacallbackerror event
  • Remove redundant console logs

Description

Improve error handling.

  • Add 'original' parameter to the 'cordovacallbackerror' event so that users can view the actual original error.
    • This is extremely important so that users can get the stack trace for logging and debugging.
  • Removed the console logs.
    • They were redundant since the error is printed when it is thrown anyways.
    • console.log's without an option to turn them off can be annoying.

Testing

npm test

  • all passed.
  • Added test for this change. (Passed)

Ran mobile spec before and after.

  • Test results were unchanged.
  • 10 cordova-plugin-file-tests failed both times. See screen shot.
    • If someone can tell me what to do to make those work I will re-run.
    • (I have the cordova-labs#cordova-filetranfer server running, and have set FILETRANSFER_SERVER_ADDRESS. This fixed a couple tests.)

Manual testing.

  • See before and after photos of default output.
  • tested window.addEventListener("cordovacallbackerror", function (err) {

before:
image

after:
image

mobilespec:
image

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

- Add 'original' parameter to the 'cordovacallbackerror' event so that users can view the actual original error.
    - This is extremely important so that users can get the stack trace for logging and debugging.
- Removed the console logs.
    - They were redundant since the error is printed when it is thrown (if not caught).
    - console.log's without an option to turn them off can be annoying.
@project-bot project-bot bot added this to 🐣 New PR / Untriaged in Apache Cordova: Tooling Pull Requests Sep 23, 2019
Lindsay-Needs-Sleep added a commit to Lindsay-Needs-Sleep/cordova-docs that referenced this issue Sep 23, 2019
@Lindsay-Needs-Sleep
Copy link
Contributor Author

@Lindsay-Needs-Sleep Lindsay-Needs-Sleep commented Sep 25, 2019

Created corresponding pull request with documentation here:
apache/cordova-docs#1018

@raphinesse raphinesse self-assigned this Oct 16, 2019
@raphinesse
Copy link
Contributor

@raphinesse raphinesse commented Oct 16, 2019

@Lindsay-Needs-Sleep Thanks a lot for your contribution and sorry for the late response! This does look like a good addition to me. I'll try to give this PR a thorough review in the following days. If I take too long, please remind me.

Maybe in the meantime @janpio can say something about the mobile spec failures. He might know a bit about that, IIRC.

- Maintain mirrored module structure between src/ and test/
- Simplify test case
@raphinesse raphinesse requested review from breautek and erisu Oct 19, 2019
Apache Cordova: Tooling Pull Requests automation moved this from 🐣 New PR / Untriaged to ✅ Approved, waiting for Merge Oct 19, 2019
Copy link
Contributor

@raphinesse raphinesse left a comment

I took the liberty to make a few adjustments to the test. I hope you are OK with them. Thanks again!

IMO, this would be good to merge. I'd like a second opinion though, if possible.

src/cordova.js Outdated
console && console.log && console.log(msg);
console && console.log && err.stack && console.log(err.stack);
cordova.fireWindowEvent('cordovacallbackerror', { 'message': msg });
cordova.fireWindowEvent('cordovacallbackerror', { 'message': msg, 'original': err });
Copy link
Member

@erisu erisu Oct 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would the property name original sound better if it is error, stack, or errorStack.

Once the property name is set and used, I am not sure if it would be changed it in the future if we find the name to be confusing.

People could build custom tooling that relays on this new field name and changing would break their tools.

Just want to make sure the meaning of the property carries over.

Copy link
Contributor

@raphinesse raphinesse Oct 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've had similar concerns. I think error would be a more meaningful property name.

Copy link
Contributor Author

@Lindsay-Needs-Sleep Lindsay-Needs-Sleep Oct 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point.

This got me thinking. What do you think of this instead?

err.message = msg;
corodva.fireWindowEvent(“cordovacallbackerror”, err);

This maintains backwards compatibility, but makes the error the top-level response.

I think this makes a lot more sense because most people will probably think the object they receive in cordovacallbackerror will be an error. (At least that is what I originally thought.)

Copy link
Contributor

@raphinesse raphinesse Oct 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That might be even better. What do you think @erisu?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm assuming in this context err is an Error object (since it comes from the catch block), assigning err.message would overwrite the original error message yes? If so, I don't think that's a good idea as that could be hiding important information for debugging. Or am I missing something?

Copy link
Contributor Author

@Lindsay-Needs-Sleep Lindsay-Needs-Sleep Oct 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@breautek Yes, you are correct. Though it is technically only prepending the error message with additional information.

In the main comment stream, @raphinesse and I have been discussing this a bit. (It is still up in the air). If you would like to, please take a look and share your thoughts on the proposals.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though it is technically only prepending the error message with additional information.

Ahh I see, that's where the + err comes from. I think this is fine.

On the topic of using error vs original. +1 on error

Using original might make sense in the context of cordovacallbackerror but when you're debugging or something you just have a reference of the object (not necessary where it is coming from), the property name original isn't too meaningful of what the property is. I think using error as a property name is much more clear.

Copy link
Contributor Author

@Lindsay-Needs-Sleep Lindsay-Needs-Sleep left a comment

(Using github from mobile kind of messed me up, ignore this!)

@Lindsay-Needs-Sleep
Copy link
Contributor Author

@Lindsay-Needs-Sleep Lindsay-Needs-Sleep commented Oct 20, 2019

Actually, I think something like this would be the nicest/cleanest option:

 } catch (err) {
    err.cordovaCallbackId = callbackId;
    err.cordovaCallbackSuccess = isSuccess;
    corodva.fireWindowEvent(“cordovacallbackerror”, err);
    throw err;
}

Pro’s:

  • This provides the information that was provided in ‘message’ in a much easier to use format.
  • It does not clobber the err’s message like my last suggestion.
  • The error is the top level object.

Cons

  • Not backwards compatible for someone was parsing the original ‘message’ string to get the callback Id and callback success state.

Reasons to do this despite the con:

  • Depending on parsing a string like that is bad practice. So likely (hopefully) very few people have done this.
  • cordovacallbackerror is completely undocumented (and untested). So, this feature is even less likely to have been used/abused.
  • If someone was parsing message, it will be a small change to fix and will result in nicer code.

Thoughts? @erisu @raphinesse

@raphinesse
Copy link
Contributor

@raphinesse raphinesse commented Oct 20, 2019

Well, the message is not exactly clobbered. We only prepend a string. Which might be helpful for someone who doesn't know what's going on.

Adding callbackId and isSuccess as props would be nice. Why not return the error with added props and amended message?

@Lindsay-Needs-Sleep
Copy link
Contributor Author

@Lindsay-Needs-Sleep Lindsay-Needs-Sleep commented Oct 20, 2019

Well, the message is not exactly clobbered. We only prepend a string. Which might be helpful for someone who doesn't know what's going on.

True, fair point.

My first thought was that we shouldn’t prepend the message incase some one was depending on the error message having an exact message. (But that it almost the exact same thing I was saying we shouldn’t worry too much about :p) But, the modified message is slightly more likely to cause an inconvenience to future people who expect an unmodified error message. This is possibly a slight concern because this error is picked by window.onerror (which quite a few more people use I assume). But this is still minor.

Since we have the stack trace included in the error, the first file reference will take you directly to where the error came from (which will be inside your success/error callback). (You can see that in my attached screenshots). This makes the majority of the prepended message redundant.

Why not return the error with added props and amended message?

But I am fine with this as well if my points didn’t sway you. :p

Adding ‘callbackId’ and ‘isSuccess’ as props would be nice.

To further decrease our odds of accidentally clobbering someone’s custom error property (unlikely, but better to be a bit safer I think), we could do this instead as well:

err.cordovaCallback = { id: cordovaCallbackId, isSuccess: isSuccess }

@raphinesse
Copy link
Contributor

@raphinesse raphinesse commented Oct 22, 2019

@Lindsay-Needs-Sleep Your points about messing with user's objects (in this case their errors) are good ones. I think with Cordova being a library, we should be as defensive as possible here. To me, that would mean:

  • return a new object (like before)
  • add the message property exactly like it was before
  • add the new error property with the original, unchanged error object
  • add callbackId
  • add a property callbackType with a value of either 'fail' or 'success'. That might be clearer than having isSuccess

@Lindsay-Needs-Sleep
Copy link
Contributor Author

@Lindsay-Needs-Sleep Lindsay-Needs-Sleep commented Oct 24, 2019

I changed the "original" parameter name to "error" as suggested.

erisu
erisu approved these changes Oct 24, 2019
@raphinesse raphinesse merged commit 7d4d671 into apache:master Oct 25, 2019
2 checks passed
Apache Cordova: Tooling Pull Requests automation moved this from ✅ Approved, waiting for Merge to 🏆 Merged, waiting for Release Oct 25, 2019
@raphinesse
Copy link
Contributor

@raphinesse raphinesse commented Oct 25, 2019

@Lindsay-Needs-Sleep Thanks again for this PR. 👍

@Lindsay-Needs-Sleep
Copy link
Contributor Author

@Lindsay-Needs-Sleep Lindsay-Needs-Sleep commented Oct 25, 2019

And thank you all for the feedback!
And you, @raphinesse, for improving the test. :)

Lindsay-Needs-Sleep added a commit to Lindsay-Needs-Sleep/cordova-docs that referenced this issue Oct 26, 2019
Lindsay-Needs-Sleep added a commit to Lindsay-Needs-Sleep/cordova-docs that referenced this issue Oct 26, 2019
raphinesse pushed a commit to apache/cordova-docs that referenced this issue Oct 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Apache Cordova: Tooling Pull Requests
🏆 Merged, waiting for Release
Development

Successfully merging this pull request may close these issues.

None yet

4 participants