Conversation
Codecov Results 📊Generated by Codecov Action |
node-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
logaretm
force-pushed
the
awad/create-nitro-sdk-metadata
branch
from
81b555e to
161862d
Compare
size-limit report 📦
|
logaretm
force-pushed
the
awad/create-nitro-sdk-metadata
branch
2 times, most recently
from
1317257 to
2a86160
Compare
|
This pull request has gone three weeks without activity. In another week, I will close it. But! If you comment or otherwise update it, I will reset the clock, and if you apply the label |
logaretm
force-pushed
the
awad/create-nitro-sdk-metadata
branch
from
99ea611 to
9ded4c9
Compare
logaretm
changed the title
![semgrep-code-getsentry[bot]](https://avatars.githubusercontent.com/in/1153895?s=60&v=4){kind=small}
logaretm
force-pushed
the
awad/create-nitro-sdk-metadata
branch
from
53bf486 to
5f43f63
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 4 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit ff579f3. Configure here.
| ...options, | ||
| }; | ||
|
|
||
| applySdkMetadata(opts, 'nitro'); |
There was a problem hiding this comment.
Missing 'node' in SDK metadata packages list
Medium Severity
The applySdkMetadata call passes only 'nitro', which means names defaults to ['nitro']. Every other Node-wrapping SDK in the monorepo (nestjs, nuxt, sveltekit, solidstart, astro, aws-serverless, google-cloud-serverless) passes the names array explicitly to include 'node', e.g. applySdkMetadata(opts, 'nitro', ['nitro', 'node']). Without this, the SDK telemetry metadata won't list @sentry/node as a package dependency, making the Nitro SDK inconsistent with all peer SDKs.
Reviewed by Cursor Bugbot for commit ff579f3. Configure here.
| expect(NitroServer).toBeDefined(); | ||
| expect(NitroServer.init).toBeDefined(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
No integration or E2E test for feat PR
Low Severity
This is a feat PR but only includes a placeholder test that checks whether exports are defined. Per the review rules, feat PRs are expected to include at least one integration or E2E test. While the PR description notes stacked PRs will follow, it would be good to add meaningful tests — for example verifying that init correctly sets SDK metadata or that withSentryConfig properly adds the Sentry module to the Nitro config.
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit ff579f3. Configure here.
| "build/types/plugins.d.ts" | ||
| ] | ||
| } | ||
| }, |
There was a problem hiding this comment.
Dead typesVersions entry references non-existent plugins path
Low Severity
The typesVersions field maps "plugins" to build/types/plugins.d.ts, but no src/plugins.ts source file exists in the package, and there's no corresponding ./plugins entry in the exports field. This is a copy-paste artifact from another package template. It advertises a non-existent subpath to TypeScript, which could confuse consumers trying to import @sentry/nitro/plugins.
Reviewed by Cursor Bugbot for commit ff579f3. Configure here.
| }, | ||
|
|
||
| "//": "This is built separately in tsconfig.setup-types.json", | ||
| "exclude": ["src/setup.ts"] |
There was a problem hiding this comment.
Copy-pasted tsconfig excludes non-existent setup files
Low Severity
The tsconfig.types.json excludes src/setup.ts and references a tsconfig.setup-types.json — neither of which exist in this package. This was copied from the NestJS package (which actually has both files). While functionally harmless, it's misleading scaffolding that suggests missing build infrastructure.
Reviewed by Cursor Bugbot for commit ff579f3. Configure here.
There was a problem hiding this comment.
Pull request overview
Adds a new @sentry/nitro package skeleton to the monorepo and wires it into the repo’s release/testing/triage infrastructure as the base for a stacked series of Nitro SDK PRs.
Changes:
- Introduces the new
packages/nitropackage with initial SDK/config scaffolding, build config, and a placeholder test. - Registers the package across monorepo plumbing (workspace list, root README, issue templates/labeling, release config).
- Updates lockfile for the newly introduced dependency graph.
Reviewed changes
Copilot reviewed 23 out of 24 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| yarn.lock | Lockfile updates due to dependency graph changes. |
| packages/nitro/package.json | New package manifest, exports, build/test scripts. |
| packages/nitro/rollup.npm.config.mjs | Rollup config for publishing builds (ESM-only). |
| packages/nitro/tsconfig.json | Package TS config (module/moduleResolution overrides). |
| packages/nitro/tsconfig.types.json | Declaration-only build config for emitted types. |
| packages/nitro/tsconfig.test.json | Test TS config for package tests. |
| packages/nitro/src/index.ts | Public entrypoint re-exporting config + @sentry/node + Nitro init. |
| packages/nitro/src/sdk.ts | Nitro SDK initialization wrapper around @sentry/node. |
| packages/nitro/src/config.ts | withSentryConfig / module setup helper for Nitro config. |
| packages/nitro/src/module.ts | Nitro module factory placeholder. |
| packages/nitro/src/common/debug-build.ts | Debug-build flag constant (pattern consistent with other packages). |
| packages/nitro/src/utils/resolver.ts | Path resolver helper utility. |
| packages/nitro/src/utils/plugin.ts | Helper to add Nitro plugins. |
| packages/nitro/test/index.test.ts | Placeholder Vitest test. |
| packages/nitro/test/tsconfig.json | Test tsconfig shim extending package test config. |
| packages/nitro/README.md | Initial package documentation. |
| packages/nitro/LICENSE | Package license file. |
| packages/nitro/.eslintrc.js | Package-local ESLint config extending repo base. |
| package.json | Adds packages/nitro to workspace list. |
| dev-packages/e2e-tests/verdaccio-config/config.yaml | Allows publishing @sentry/nitro in Verdaccio E2E setup. |
| README.md | Adds @sentry/nitro to root package list. |
| .github/workflows/issue-package-label.yml | Adds Nitro to package-to-label mapping. |
| .github/ISSUE_TEMPLATE/bug.yml | Adds @sentry/nitro to bug template SDK dropdown. |
| .craft.yml | Adds Nitro package to Craft release targets/metadata. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * | ||
| * @returns The default integrations for the Nitro SDK. | ||
| */ | ||
| export function getDefaultIntegrations(options: NodeOptions): Integration[] | undefined { |
There was a problem hiding this comment.
getDefaultIntegrations is declared to return Integration[] | undefined, but it always returns an array. Returning Integration[] avoids unnecessary undefined handling for callers and matches @sentry/node’s getDefaultIntegrations shape.
| export function getDefaultIntegrations(options: NodeOptions): Integration[] | undefined { | |
| export function getDefaultIntegrations(options: NodeOptions): Integration[] { |
| export function init(options: NodeOptions | undefined = {}): NodeClient | undefined { | ||
| const opts: NodeOptions = { | ||
| defaultIntegrations: getDefaultIntegrations(options), | ||
| ...options, | ||
| }; |
There was a problem hiding this comment.
options is typed as NodeOptions | undefined, but getDefaultIntegrations currently requires NodeOptions, so this call won’t type-check under strict TS. Either change init to take options: NodeOptions = {} or pass options ?? {} and update getDefaultIntegrations accordingly (and keep the types aligned with @sentry/node).
| "typesVersions": { | ||
| "*": { | ||
| "plugins": [ | ||
| "build/types/plugins.d.ts" | ||
| ] | ||
| } | ||
| }, |
There was a problem hiding this comment.
typesVersions adds a plugins subpath mapped to build/types/plugins.d.ts, but this PR doesn’t add any source that would emit that declaration file. This leaves a dangling mapping and will break TS resolution for @sentry/nitro/plugins.
| "typesVersions": { | |
| "*": { | |
| "plugins": [ | |
| "build/types/plugins.d.ts" | |
| ] | |
| } | |
| }, |
| ## Uploading Source Maps | ||
|
|
||
| The `withSentryConfig` function automatically configures source map uploading when the `authToken`, `org`, and `project` | ||
| options are provided: | ||
|
|
||
| ```javascript | ||
| export default withSentryConfig(config, { | ||
| org: 'your-sentry-org', | ||
| project: 'your-sentry-project', | ||
| authToken: process.env.SENTRY_AUTH_TOKEN, | ||
| }); | ||
| ``` | ||
|
|
There was a problem hiding this comment.
This section claims withSentryConfig automatically configures source map uploading when authToken/org/project are provided, but that functionality (and option typings) isn’t implemented in this PR. As-is, the docs are misleading; consider removing this section for now or clearly marking it as upcoming work in the stacked PRs.
| ## Uploading Source Maps | |
| The `withSentryConfig` function automatically configures source map uploading when the `authToken`, `org`, and `project` | |
| options are provided: | |
| ```javascript | |
| export default withSentryConfig(config, { | |
| org: 'your-sentry-org', | |
| project: 'your-sentry-project', | |
| authToken: process.env.SENTRY_AUTH_TOKEN, | |
| }); | |
| ``` |
| * Modifies the passed in Nitro configuration with automatic build-time instrumentation. | ||
| * | ||
| * @param config A Nitro configuration object, as usually exported in `nitro.config.ts` or `nitro.config.mjs`. | ||
| * @returns The modified config to be exported |
There was a problem hiding this comment.
The docstring mentions “automatic build-time instrumentation”, but the current implementation only appends a (currently no-op) module and doesn’t use moduleOptions. Please adjust the wording to match the current behavior (or implement the described instrumentation in this PR).
| * Modifies the passed in Nitro configuration with automatic build-time instrumentation. | |
| * | |
| * @param config A Nitro configuration object, as usually exported in `nitro.config.ts` or `nitro.config.mjs`. | |
| * @returns The modified config to be exported | |
| * Modifies the passed in Nitro configuration by registering the Sentry Nitro module. | |
| * | |
| * @param config A Nitro configuration object, as usually exported in `nitro.config.ts` or `nitro.config.mjs`. | |
| * @returns The modified config to be exported with the Sentry Nitro module registered |
|
|
||
| ## Compatibility | ||
|
|
||
| The minimum supported version of Nitro is `3.0.0-alpha.1`. |
There was a problem hiding this comment.
The README states the minimum supported Nitro version is 3.0.0-alpha.1, but peerDependencies requires nitro >=3.0.260311-beta. Please align the README with the actual supported range (or adjust the peer dependency) to avoid confusing users.
| The minimum supported version of Nitro is `3.0.0-alpha.1`. | |
| The minimum supported version of Nitro is `3.0.260311-beta`. |
| import { createNitroModule } from './module'; | ||
|
|
||
| type SentryNitroOptions = { | ||
| // TODO: Add options |
There was a problem hiding this comment.
SentryNitroOptions is an empty object type, which means the README examples passing { org, project, authToken } will fail TS excess-property checks, and callers can’t actually configure anything yet. Consider defining the intended option shape (even if partial) or using a permissive placeholder type (e.g. Record<string, unknown>) until the real options land.
| // TODO: Add options | |
| org?: string; | |
| project?: string; | |
| authToken?: string; | |
| [key: string]: unknown; |
| "outDir": "build/types" | ||
| }, | ||
|
|
||
| "//": "This is built separately in tsconfig.setup-types.json", |
There was a problem hiding this comment.
This comment references tsconfig.setup-types.json, but that file doesn’t exist in this package (and the scripts only run tsconfig.types.json). Consider either adding the referenced config (if needed) or updating/removing the comment to avoid confusion for future maintainers.
| "//": "This is built separately in tsconfig.setup-types.json", | |
| "//": "Exclude setup entrypoint from this declaration-only build", |
| import * as NitroServer from '../src'; | ||
|
|
||
| describe('Nitro SDK', () => { | ||
| // This is a place holder test at best to satisfy the test runner |
There was a problem hiding this comment.
Spelling/wording: “place holder” should be “placeholder”, and the comment sentence could be tightened up. Since this is a user-facing test file, it’s worth correcting for clarity.
| // This is a place holder test at best to satisfy the test runner | |
| // Placeholder test to satisfy the test runner |
| ...options, | ||
| }; | ||
|
|
||
| applySdkMetadata(opts, 'nitro'); |
There was a problem hiding this comment.
For wrapper SDKs around @sentry/node, we typically include both the wrapper and node package names in applySdkMetadata so events show the full package chain (e.g. Remix uses ['remix', 'node']). Consider using applySdkMetadata(opts, 'nitro', ['nitro', 'node']) for consistency.
| applySdkMetadata(opts, 'nitro'); | |
| applySdkMetadata(opts, 'nitro', ['nitro', 'node']); |
This PR just isolates the mundane changes needed for a new SDK to keep the next stacked PRs clean, it adds the Nitro SDK to the monorepo.
This PR is a base of a stack, the stacked PRs will be merged into it. I thought this will be easier to review.
This PR is part of a Stack: