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(react): Add a handled prop to ErrorBoundary #14560

Merged

Conversation

HHK1
Copy link
Contributor

@HHK1 HHK1 commented Dec 3, 2024

The previous behaviour was to rely on the presence of the fallback prop to decide if the error was considered handled or not. The new property lets the consumer explicitely choose what should the handled status be.
If omitted, the old behaviour is still applied.

Before submitting a pull request, please take a look at our
Contributing guidelines and verify:

  • If you've added code that should be tested, please add tests.
  • Ensure your code lints and the test suite passes (yarn lint) & (yarn test).

-> I'm having some failures locally that seem unrelated to my changes. Waiting for a CI check to confirm

@HHK1
Copy link
Contributor Author

HHK1 commented Dec 3, 2024

@Lms24 not sure what to do regarding the CI.

build works fine locally, but I'm having errors when running lint and test. It seems very much unrelated to the changes here.
Any advices on making the local setup work?

Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

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

Hey @HHK1 thanks for opening this PR! The proposed changes look good to me! I had some suggestions to streamline the implementation but looks fine! I'll start CI to check for the linting stuff but it most definitely will fail because there's still an .only filter in the tests (we have a lint rule against accidentally merging this in).

I'd generally recommend to run yarn fix:biome in the root directory of the repo to fix any formatting and biome lint issues.

I can take another look about remaining CI fails if things still fail after the changes.

Comment on lines 116 to 117
const isHandled = this.props.handled === undefined ? !!this.props.fallback : this.props.handled;
const eventId = captureReactException(error, errorInfo, { mechanism: { handled: isHandled } });
Copy link
Member

Choose a reason for hiding this comment

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

a suggestion to make this a bit more concise and save some bytes of bundle size. I turned around the condition to reflect that the fallback is actually our fallback way of determining handled:

Suggested change
const isHandled = this.props.handled === undefined ? !!this.props.fallback : this.props.handled;
const eventId = captureReactException(error, errorInfo, { mechanism: { handled: isHandled } });
const handled = this.props.handled != null ? this.props.handled : !!this.props.fallback;
const eventId = captureReactException(error, errorInfo, { mechanism: { handled } });

(!= null can be used as a shorthand for !== undefined && !== null)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 11c408d

packages/react/test/errorboundary.test.tsx Outdated Show resolved Hide resolved
@@ -537,47 +537,47 @@ describe('ErrorBoundary', () => {
expect(mockOnReset).toHaveBeenCalledTimes(1);
expect(mockOnReset).toHaveBeenCalledWith(expect.any(Error), expect.any(String), expect.any(String));
});
it.only.each`
Copy link
Member

Choose a reason for hiding this comment

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

This we should most definitely remove :)

Suggested change
it.only.each`
it.each`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in 9c1cf83

@HHK1
Copy link
Contributor Author

HHK1 commented Dec 4, 2024

@Lms24 thanks for the review!

I've pushed a couple fixups, once the conversations are resolved I'll squash them to have a single commit.

I ran the formatting through the VSCode extension on the files I've touched, but I got the following kind of issues when running yarn run lint locally at the root of the repo:

Formatter would have printed the following content:

    3 3 │   import type { RouterType } from './server/solidrouter';
    4 4 │   export declare function withSentryRouterRouting(Router: RouterType): RouterType;
    5   │ - //#·sourceMappingURL=solidrouter.d.ts.map
      5 │ + //#·sourceMappingURL=solidrouter.d.ts.map
      6 │

The yarn test command is also failing:

    FAIL  test/reactrouterv3.test.tsx
        ● Test suite failed to run

          test/reactrouterv3.test.tsx:21:16 - error TS2403: Subsequent variable declarations must have the same type.  Variable 'Router' must be of type 'typeof import("/Users/henryhuck/Documents/oss/sentry-javascript/node_modules/react-router/lib/Router")', but here has type 'ComponentType<{ history: History; }>'.

          21   export const Router: React.ComponentType<{ history: History }>;

It does seem like a local setup error. Rings any bell? Not a big deal but I was just curious as why it could fail.

@HHK1 HHK1 requested a review from Lms24 December 4, 2024 13:57
@HHK1 HHK1 force-pushed the hhk/react-boundary-explicit-handled-prop branch from 11c408d to c1e5bcc Compare December 4, 2024 14:07
@HHK1
Copy link
Contributor Author

HHK1 commented Dec 9, 2024

@Lms24 just a soft ping if you could have a look 😇🙏
I think everything is good now, let me know if you want me to squash the fixups before re-running the CI (I use them to make the changes clearer to re-review).

HHK1 added 3 commits January 10, 2025 10:49
The previous behaviour was to rely on the presence of the `fallback`
prop to decide if the error was considered handled or not.
The new property lets the consumer explicitely choose what should the
handled status be.
If omitted, the old behaviour is still applied.
@HHK1 HHK1 force-pushed the hhk/react-boundary-explicit-handled-prop branch from c1e5bcc to 1a46add Compare January 10, 2025 09:49
@HHK1
Copy link
Contributor Author

HHK1 commented Jan 10, 2025

@Lms24 👋 would it be possible to have a review on this please?

Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

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

Apologies for the late review! This looks good to me now, thanks for contributing and for making the requested changes :)

Heads-up: I'll cherry pick this commit once it's merged onto our v8 branch to ensure we can include this in the next v8 release, probably sometime next week.

@Lms24 Lms24 changed the title feat(react): add a handled prop to ErrorBoundary feat(react): Add a handled prop to ErrorBoundary Jan 10, 2025
@Lms24 Lms24 assigned Lms24 and unassigned Lms24 Jan 10, 2025
@Lms24 Lms24 enabled auto-merge (squash) January 10, 2025 11:32
@Lms24 Lms24 merged commit ac6ac07 into getsentry:develop Jan 10, 2025
64 checks passed
Lms24 pushed a commit that referenced this pull request Jan 10, 2025
The previous behaviour was to rely on the presence of the `fallback`
prop to decide if the error was considered handled or not. The new
property lets users explicitly choose what should the handled
status be. If omitted, the old behaviour is still applied.
Lms24 added a commit that referenced this pull request Jan 10, 2025
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.

2 participants