Catch exceptions when accessing FullStory API methods #58
Conversation
Catch exceptions when accessing FS API methods.
|
Pinging @scefali, contributor is looking to get a PR reviewed. |
# Conflicts: # package.json # src/SentryFullStory.ts
greg-wb-ap
changed the title
|
Any action required to get this merged? |
|
@greg-wb-ap I see that you upgraded/edited some of the dependencies. We generally try to keep those to their own PRs, so would you mind revising this? Just functionality changes would make it an easier review/approval 😄 |
# Conflicts: # src/SentryFullStory.ts
|
@leeandher I have removed the dependency changes from this PR. Thanks. |
There was a problem hiding this comment.
Looks good, just a few follow up questions @greg-wb-ap. Sorry for the delays!
src/SentryFullStory.ts
Outdated
| if (e instanceof Error) { | ||
| return `Unable to get url: ${e.message}` | ||
| } | ||
| return 'Unable to get url' |
There was a problem hiding this comment.
Is this check necessary? e.message should always exist right, since the param e in the catch block is always an error?
There was a problem hiding this comment.
My understanding is you can throw anything in Javascript (it's just discouraged to not throw Error).
There was a problem hiding this comment.
That's interesting, good catch! I think we're safe to assume it'll be an Error though so we can just return with the error message.
There was a problem hiding this comment.
Typescript complains if I try to access message without performing a typecheck - I have simplified this code a little though.
| } catch (e) { | ||
| console.debug('Unable to report sentry error details to FullStory') | ||
| } |
There was a problem hiding this comment.
Is there a reason this is console.debug vs console.error?
There was a problem hiding this comment.
I tend to only use console.error for things that result in user facing issues & this isn't a particularly informative message.
Happy to consider changes here if you have a suggestion.
There was a problem hiding this comment.
No suggestion, just curious for the reasoning, looks fine to me.
src/SentryFullStory.ts
Outdated
| if (e instanceof Error) { | ||
| return `Unable to get url: ${e.message}` | ||
| } | ||
| return 'Unable to get url' |
There was a problem hiding this comment.
That's interesting, good catch! I think we're safe to assume it'll be an Error though so we can just return with the error message.
| } catch (e) { | ||
| console.debug('Unable to report sentry error details to FullStory') | ||
| } |
There was a problem hiding this comment.
No suggestion, just curious for the reasoning, looks fine to me.
|
Sorry for the long review process, thanks for being patient! We'll batch this patch with another open PR for release once we get that reviewed. |
Catch exceptions when accessing FS API methods, as otherwise the actual error that occurred gets swallowed by the FS exception.
Also upgraded some dependencies (rollup being the main one).