-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
chore(tanstackstart-react)!: Remove empty placeholder implementations #18338
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
Conversation
| return props.children; | ||
| }; | ||
|
|
||
| /** | ||
| * A passthrough redux enhancer for the server that doesn't depend on anything from the `@sentry/react` package. | ||
| */ | ||
| export function createReduxEnhancer() { | ||
| return (createStore: unknown) => createStore; | ||
| } | ||
|
|
||
| /** | ||
| * A passthrough error boundary wrapper for the server that doesn't depend on any react. Error boundaries don't catch | ||
| * SSR errors so they should simply be a passthrough. |
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.
Bug: createReduxEnhancer() and showReportDialog() are declared in types but are undefined at runtime on the server, causing TypeError.
Severity: CRITICAL | Confidence: High
🔍 Detailed Analysis
The PR removes createReduxEnhancer() and showReportDialog() from packages/tanstackstart-react/src/server/index.ts. However, their type declarations persist in packages/tanstackstart-react/src/index.types.ts. This allows TypeScript to permit calls to Sentry.createReduxEnhancer() and Sentry.showReportDialog() on the server. At runtime, these functions are undefined because they are no longer exported, leading to a TypeError: ... is not a function if invoked.
💡 Suggested Fix
Remove or update the type declarations for createReduxEnhancer() and showReportDialog() in packages/tanstackstart-react/src/index.types.ts to align with their removal from server/index.ts.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: packages/tanstackstart-react/src/server/index.ts#L16-L21
Potential issue: The PR removes `createReduxEnhancer()` and `showReportDialog()` from
`packages/tanstackstart-react/src/server/index.ts`. However, their type declarations
persist in `packages/tanstackstart-react/src/index.types.ts`. This allows TypeScript to
permit calls to `Sentry.createReduxEnhancer()` and `Sentry.showReportDialog()` on the
server. At runtime, these functions are `undefined` because they are no longer exported,
leading to a `TypeError: ... is not a function` if invoked.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 3982457
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.
Bug: Breaking change: removed publicly exported server-side functions (Bugbot Rules)
Flagging this because the review rules mention to flag removal of publicly exported functions. The createReduxEnhancer and showReportDialog functions are removed from the server module, but they're still declared as exports in index.types.ts. Since @sentry/node doesn't export these functions, isomorphic code importing them from @sentry/tanstackstart-react will get runtime errors on the server while TypeScript indicates they exist. The previous passthrough implementations allowed these functions to be safely called in code running on both client and server.
packages/tanstackstart-react/src/server/index.ts#L17-L28
sentry-javascript/packages/tanstackstart-react/src/server/index.ts
Lines 17 to 28 in 3c402fe
| }; | |
| /** | |
| * A passthrough error boundary wrapper for the server that doesn't depend on any react. Error boundaries don't catch | |
| * SSR errors so they should simply be a passthrough. | |
| */ | |
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | |
| export function withErrorBoundary<P extends Record<string, any>>( | |
| WrappedComponent: React.ComponentType<P>, | |
| ): React.FC<P> { | |
| return WrappedComponent as React.FC<P>; | |
| } |
s1gr1d
left a comment
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.
Since it's alpha, we can break that (as you already wrote). I added a ! to the title to mark it as breaking.
Just cleaning up, we won't be needing any of these. I am aware this is breaking, but I think it's fine since we are still in alpha and these are empty anyways.