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

feat(nestjs): Add @sentry/nestjs #12613

Merged
merged 34 commits into from
Jun 26, 2024
Merged

feat(nestjs): Add @sentry/nestjs #12613

merged 34 commits into from
Jun 26, 2024

Conversation

nicohrubec
Copy link
Contributor

@nicohrubec nicohrubec commented Jun 24, 2024

@nicohrubec nicohrubec marked this pull request as ready for review June 25, 2024 08:35
/**
* Initializes the NestJS SDK
*/
export function init(options: NodeOptions | undefined = {}): void {
Copy link
Member

Choose a reason for hiding this comment

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

h: SDKs are supposed to return the Client

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

</a>
</p>

# Official Sentry SDK for NestJS
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# Official Sentry SDK for NestJS
# Official Sentry SDK for NestJS (EXPERIMENTAL)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated the readme to indicate that it is experimental

Copy link
Member

Choose a reason for hiding this comment

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

I would still put the change Andrei suggested. It's better to be very loud with it than people potentially breaking because they missed it.

Copy link
Member

Choose a reason for hiding this comment

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

Yea, the Nuxt and the Solid sdk both follow this "convention" now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, added

@@ -0,0 +1 @@
module.exports = require('../../jest/jest.config.js');
Copy link
Member

Choose a reason for hiding this comment

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

Hm, wondering if we shouldn't use vitest instead.
cc @mydea @lforst

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I like that. @nicohrubec would you mind adding a vitest setup instead of jest. You can take inspiration from the svelte, sveltekit and solid packages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -0,0 +1,772 @@
# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.
Copy link
Member

@andreiborza andreiborza Jun 25, 2024

Choose a reason for hiding this comment

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

m: You can remove this file, it was probably generated before you added the package to the workspace.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for the hint, removed it

Comment on lines 50 to 52
"optionalDependencies": {
"opentelemetry-instrumentation-fetch-node": "1.2.0"
},
Copy link
Member

Choose a reason for hiding this comment

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

Let's get rid of this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

@lforst lforst changed the title Add nest js basic package feat(nestjs): Add @sentry/nestjs Jun 25, 2024
# Official Sentry SDK for NestJS

This SDK is considered **experimental and in an alpha state**. It may experience breaking changes. Please reach out on
[GitHub](https://github.com/getsentry/sentry-javascript/issues/new/choose) if you have any feedback or concerns.
Copy link
Member

Choose a reason for hiding this comment

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

I think we should flesh out the readme a bit more. Ideally we get it to a similar state as the readme of the @sentry/node package with basic setup instructions and how to use. Once we add more features to this package we can update this readme again.

andreiborza
andreiborza previously approved these changes Jun 25, 2024
Copy link
Member

@andreiborza andreiborza left a comment

Choose a reason for hiding this comment

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

Good stuff!

@lforst lforst dismissed andreiborza’s stale review June 25, 2024 13:59

dismissing because some critical things are still missing

@@ -0,0 +1,71 @@
{
"name": "@sentry/nestjs",
"version": "8.11.0",
Copy link
Member

Choose a reason for hiding this comment

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

This should be 8.12.0 now right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

"author": "Sentry",
"license": "MIT",
"engines": {
"node": ">=14.18"
Copy link
Member

Choose a reason for hiding this comment

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

We should align this with the minimum supported version of nest since we also don't test for 14.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@nicohrubec nicohrubec merged commit 8c94548 into develop Jun 26, 2024
113 checks passed
@nicohrubec nicohrubec deleted the nh/nest-sdk branch June 26, 2024 11:09
@nicohrubec nicohrubec mentioned this pull request Jul 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants