Skip to content
This repository was archived by the owner on Mar 17, 2025. It is now read-only.

Conversation

dreadjr
Copy link
Contributor

@dreadjr dreadjr commented Jun 11, 2015

Wanted to start a discussion about 2 things,

  1. adding error_stack
  2. reject new Error() vs string

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@dreadjr
Copy link
Contributor Author

dreadjr commented Jun 11, 2015

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

@cbraynor
Copy link
Contributor

I'll take a look at this today - initially I used strings because they are easiest to represent in Firebase, and perhaps we don't want to coerce every rejection with a string into an Error object, but a stack trace would be a useful addition

@dreadjr
Copy link
Contributor Author

dreadjr commented Jun 12, 2015

Ok cool, yeah i wasn't sure about the error stack as far as privacy/security is concerned either. It might need to go in an "errors" bucket and just reference it. I was thinking could just do a JSON.stringify as well.

@cbraynor
Copy link
Contributor

In using the queue, I've tended to lock down the /tasks location to write only (and create only) for clients that push items onto the queue, and only allowed the trusted server processes read access. This means that for me, adding the stack trace to the task is a non-issue.

I'm curious whether that's a pattern other people are using, or whether read access is indeed required in some use-cases? If read access is required, then you're right that adding stack traces could be a security concern

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure how useful this being an error would be - won't the stack trace always be an internal Firebase Queue node module trace? Won't they be able to get that information from the error message string itself?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, sorry, was confused for a second between reject and _reject - this could be useful

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Event though the stack trace might not be helpful in all cases, it's nice just to be consistent. Also might help later on if more of the internal api is exposed and can be built upon.

@dreadjr
Copy link
Contributor Author

dreadjr commented Jun 13, 2015

Yeah true. I was thinking too of converting stack trace into Jain so it could be saved to firebase more easily.

@cbraynor
Copy link
Contributor

I included these changes in #24 with a few minor adjustments. The major changes were

  1. We don't specifically coerce all errors to Error objects - if they're strings then they behave the same as they do now (without a stack trace)
  2. There's now a suppressStack option for the queue that prevents stack traces from ending up in the Firebase

Take a look and let me know if there's something I missed. Let's move the conversation there

@cbraynor cbraynor closed this Jun 15, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants