-
Notifications
You must be signed in to change notification settings - Fork 29
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
chore: Version 1.0.0 #233
chore: Version 1.0.0 #233
Conversation
* rename legacy send method to sendWithCallback * implemented async send wrapper * add async send test suite * update domain example to use async send * prettier run * cleanup * fix test * wip updating expressHandler to use async send * fixing tests * format * replace sendWithCallback to send * wip update docs * cleanup of sendSync * add TODOs for refactor * update README * prettier * add send bind again * cleanup test * clenaup tests * change title * Update lib/raygun.ts Co-authored-by: Sumitra Manga <[email protected]> * wrap HTTP request in Promise * fix: #198 reject promise on error * cleanup * adapt batch transport to transport with Promise * tests are passing * tests are passing * WIP * fix test * fixes and cleanup * add tags to debug loglines * improve logs in raygun.transport * fix: #140 throw error on incoming message too large * improving error logs * error cleanup * package-lock.json changes --------- Co-authored-by: Sumitra Manga <[email protected]>
Bumps [semver](https://github.com/npm/node-semver) from 7.6.0 to 7.6.2. - [Release notes](https://github.com/npm/node-semver/releases) - [Changelog](https://github.com/npm/node-semver/blob/main/CHANGELOG.md) - [Commits](npm/node-semver@v7.6.0...v7.6.2) --- updated-dependencies: - dependency-name: semver dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [@types/node](https://github.com/DefinitelyTyped/DefinitelyTyped/tree/HEAD/types/node) from 20.12.8 to 20.12.11. - [Release notes](https://github.com/DefinitelyTyped/DefinitelyTyped/releases) - [Commits](https://github.com/DefinitelyTyped/DefinitelyTyped/commits/HEAD/types/node) --- updated-dependencies: - dependency-name: "@types/node" dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [tap](https://github.com/tapjs/tapjs) from 18.7.2 to 18.8.0. - [Release notes](https://github.com/tapjs/tapjs/releases) - [Commits](https://github.com/tapjs/tapjs/compare/[email protected]@18.8.0) --- updated-dependencies: - dependency-name: tap dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
) Bumps [@stylistic/eslint-plugin](https://github.com/eslint-stylistic/eslint-stylistic/tree/HEAD/packages/eslint-plugin) from 2.0.0 to 2.1.0. - [Release notes](https://github.com/eslint-stylistic/eslint-stylistic/releases) - [Changelog](https://github.com/eslint-stylistic/eslint-stylistic/blob/main/CHANGELOG.md) - [Commits](https://github.com/eslint-stylistic/eslint-stylistic/commits/v2.1.0/packages/eslint-plugin) --- updated-dependencies: - dependency-name: "@stylistic/eslint-plugin" dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
* setup eslint style * rules for quotes * add eslint plugin ts * wip configs * wip * add rules * define rules * add comment * add implementation from old breadcrumbs branch * tests passing * add todo * WIP * cleaning up and adding docs * scoping breadcrumbs in error handler * improving tests * cleanup tests * fix eslint * prettier * moved method * add Breadcrumb object test * add Breadcrumb object test * fix test * expanded the express-example * prettier * prettier * add breadcrumbs to domains app * cleanup and test for clear breadcrumbs * prettier * rename * refactor debug statements * rename logger.js to raygun.client.js * prettier * rename * document Breadcrumbs and rename Breadcrumb type * improve docs * add word * removed commented out code
* typo * fix favicon in express-sample * import only addRequestBreadcrumbs method * add comments on types.ts for breadcrumbs * update indentation config json * fix indentation * uncomment mistake * Change Raygun.io to Raygun.com * prettier fix * Update lib/types.ts Co-authored-by: Sumitra Manga <[email protected]> --------- Co-authored-by: Sumitra Manga <[email protected]>
Bumps [@types/node](https://github.com/DefinitelyTyped/DefinitelyTyped/tree/HEAD/types/node) from 20.12.11 to 20.12.12. - [Release notes](https://github.com/DefinitelyTyped/DefinitelyTyped/releases) - [Commits](https://github.com/DefinitelyTyped/DefinitelyTyped/commits/HEAD/types/node) --- updated-dependencies: - dependency-name: "@types/node" dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [@eslint/js](https://github.com/eslint/eslint/tree/HEAD/packages/js) from 9.2.0 to 9.3.0. - [Release notes](https://github.com/eslint/eslint/releases) - [Changelog](https://github.com/eslint/eslint/blob/main/CHANGELOG.md) - [Commits](https://github.com/eslint/eslint/commits/v9.3.0/packages/js) --- updated-dependencies: - dependency-name: "@eslint/js" dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [typescript-eslint](https://github.com/typescript-eslint/typescript-eslint/tree/HEAD/packages/typescript-eslint) from 7.8.0 to 7.9.0. - [Release notes](https://github.com/typescript-eslint/typescript-eslint/releases) - [Changelog](https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/typescript-eslint/CHANGELOG.md) - [Commits](https://github.com/typescript-eslint/typescript-eslint/commits/v7.9.0/packages/typescript-eslint) --- updated-dependencies: - dependency-name: typescript-eslint dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
* chore: Prepare Release 1.0.0 * chore: update changelog * add breaking change note to changelog * sorted changelog
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.
I've tested and code reviewed the majority! LGTM, nice work 🚀
@sumitramanga should I wait for more reviews to merge and publish? |
Waiting on Panos to review first ! @miquelbeltran |
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.
My main concern is introducing breaking changes. I'd like us to consider adding deprecated, backwards-compatible functions if possible. Let's discuss to reach a consensus on this :-)
Apart from that, great work!! I can see a lot was done!
- chore: code cleanup (#225) (2024-05-20) | ||
- chore: apply prettier to all files in examples (#226) (2024-05-19) | ||
|
||
**BREAKING CHANGES** |
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.
Why don't we support a backwards-compatible but deprecated (and to be removed next) send
method? With some additional checking logic, it could as simple as function send(error, customData={}, request,={} tags={}){ return send(error, {customData: customData, request: request, tags: tags});}
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.
If it makes sense to add in the deprecation, we could do this now whilst we discuss the final decision. It looks like we've done something in the past about deprecating (which we may need to remove -
Line 159 in 8e37c10
// This function is deprecated, is provided for legacy apps and will be |
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.
We provide a backwards compatible method, is sendWithCallback()
.
Lines 292 to 318 in 8e37c10
/** | |
* @deprecated sendWithCallback is a deprecated method. Instead, use send, which supports async/await calls. | |
*/ | |
sendWithCallback( | |
exception: Error | string, | |
customData?: CustomData, | |
callback?: Callback<IncomingMessage>, | |
request?: RequestParams, | |
tags?: Tag[], | |
) { | |
// call async send but redirect response to provided legacy callback | |
this.send(exception, { | |
customData, | |
request, | |
tags, | |
}) | |
.then((response) => { | |
if (callback) { | |
callVariadicCallback(callback, null, response); | |
} | |
}) | |
.catch((error) => { | |
if (callback) { | |
callVariadicCallback(callback, error, null); | |
} | |
}); | |
} |
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 could as simple as
function send(error, customData={}, request,={} tags={}){ return send(error, {customData: customData, request: request, tags: tags});}
JavaScript doesn't support overloading methods. There ware some workarounds but have limitations.
README.md
Outdated
|
||
Follow the instructions on https://raygun.com/platform/apm to set up your application. | ||
|
||
Then, call to `setApmBridge()` to connect both packages: |
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.
Add a full example of how this works. Something like:
require('raygun-apm/http'); const raygun = require('raygun'); raygun.setApmBridge();
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 is already in examples/apm-sample/app.js
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.
Ah you mean in the README, got it.
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.
But I think the current text is correct. In other parts of the readme, we don't reference to the require
for raygun client, and also the require('raygun-apm/http')
is not required to set up the APM bridge. I also link to the official APM setup process.
**BREAKING CHANGES** | ||
|
||
- `send()` method signature changed. | ||
- APM Bridge setup process changed. |
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.
What were we doing before and as above, can we support a deprecated version?
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.
The previous setup process was causing compilation errors to some users and was not possible to properly unit test it, it is explained in detail here: #222
README.md
Outdated
Then, call to `setApmBridge()` to connect both packages: | ||
|
||
```js | ||
raygunClient.setApmBridge(require("raygun-apm/lib/src/crash_reporting")); |
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.
That looks like a nasty setup. Can we make it without arguments and infer it? Still have the explicit call with the argument as fallback
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 you take a look at the PR where we introduced this and review it? #222
examples/apm-sample/README.md
Outdated
|
||
To do so, navigate to the project root directory, then: | ||
|
||
npm install |
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.
A customer yesterday was asking if we support pnpm
instead. Investigate and please add to the docs as an alternative
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.
Let's make this an issue to resolved outside of this release
examples/apm-sample/app.js
Outdated
@@ -0,0 +1,25 @@ | |||
var config = require("config"); | |||
|
|||
if (config.Raygun.Key === "YOUR_API_KEY") { |
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 we be encouraging no API keys in code to set a good standard? Could be via .env
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.
Because this is an example app, I would think this is not necessary to add in
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.
raygun-apm
gets the api key from env variable, which we should probably do as well eventually, out of scope for this release IMO.
|
||
/** | ||
* Get port from environment and store in Express. | ||
*/ | ||
|
||
var port = normalizePort(process.env.PORT || '3000'); | ||
app.set('port', port); | ||
var port = normalizePort(process.env.PORT || "3000"); |
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.
What if the user does not use process.env.PORT
to set up Express? Should we be pulling this elsewhere, if possible?
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.
Because this is an example app, we should not have to worry about this? 🤔 We are using the standard way to set up ports I would think. Happy to be challenged! Such as, what other way would people use commonly :)
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.
process.env.PORT || "3000"
works that if the user doesn't have a $PORT
in their env vars, 3000
will be used by default.
Yes, agreed with @sumitramanga I guess this is standard express:
@PanosNB I will do a new PR with some of the suggestions. I've also replied on Slack regarding the discussion on breaking changes in the Please take a look at my other replies in your review, thanks! |
* docs: update send params in README * docs: add Breadcrumbs info * docs: change should by must
APM Bridge changes reverted |
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.
Thanks Miguel, great work!
Version 1.0.0
Description 📝
Type of change
Updates
Changelog:
raygun.transport.ts
to use Promises instead of callbaks #197 Refactor to use Promises internally (refactor: #197 Refactor to use Promises internally #200) (2024-05-09)Test plan 🧪
Author to check 👓
Reviewer to check ✔️