-
Notifications
You must be signed in to change notification settings - Fork 251
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
V7: rename "report" to "event" #646
Conversation
5ee7623
to
d65f902
Compare
This prerelease dependency which is not in git tree meant these packages got installed from npm rather than symlinking locally – meaning the tests broke after the refactor.
d65f902
to
cba9d08
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There looks to be a couple more occurrences of report in the examples that might need looking at e.g. the line "you can record all kinds of events which will be sent along with error reports".
sendReport: (report) => { | ||
const evt = report.events[0] | ||
sendEvent: (event) => { | ||
const evt = event.events[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can an event can contain sub-events or is this a kind of naming conflict?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's a poor choice of name – the argument should be called payload
@@ -20,7 +20,7 @@ | |||
"author": "Bugsnag", | |||
"license": "MIT", | |||
"devDependencies": { | |||
"@bugsnag/core": "^7.0.0-pre-alpha-ben.6", | |||
"@bugsnag/core": "^6.4.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be part of this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah the explanation is in this commit: c342609
It was needed to ensure the tests pass.
@@ -20,7 +20,7 @@ | |||
"author": "Bugsnag", | |||
"license": "MIT", | |||
"devDependencies": { | |||
"@bugsnag/core": "^7.0.0-pre-alpha-ben.6", | |||
"@bugsnag/core": "^6.4.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be part of this PR?
packages/plugin-react-native-unhandled-rejection/test/rejection-handler.test.js
Outdated
Show resolved
Hide resolved
I left these in on purpose – we still "report errors" (the spec even has a section called "reporting errors") – it's just the construct in code which has changed. I'll address all your other comments though, they're useful 👍 |
- disambiguate event/payload sendEvent argument in tests - fix grammar - fix overzealous find and replaces
8a04f05
to
9b1873f
Compare
The construct represented by the "report" class in this codebase is now called "event" in the spec. This also now matches up with the concept of an "event" in our dashboard and other areas of the product.
There are changes to be made to its interface, but these will be included in subsequent PRs.
No functionality should have been changed here, literally just the naming.