Skip to content

Conversation

mydea
Copy link
Member

@mydea mydea commented Sep 17, 2025

I've seen a few places where we are wrapping things that could be sync or async, and we want to do something with the return value. By adding an onSuccess handler to handelCallbackErrors we can handle this more generically in the future:

const wrapped = handleCallbackErrors(
  () => fn(), 
  // error handler
  (_err) => {}, 
  // finally handler
  () => {},
  // success handler, gets value or resolved value if function returns a promise
  (_value) => {}
);

While scanning a bit for places where we could already use this, I also found two bugs around this, opened separate PRs for them:

cursor[bot]

This comment was marked as outdated.

Copy link
Contributor

github-actions bot commented Sep 17, 2025

size-limit report 📦

⚠️ Warning: Base artifact is not the latest one, because the latest workflow run is not done yet. This may lead to incorrect results. Try to re-run all tests to get up to date results.

Path Size % Change Change
@sentry/browser 24.16 kB - -
@sentry/browser - with treeshaking flags 22.72 kB - -
@sentry/browser (incl. Tracing) 40.17 kB +0.03% +11 B 🔺
@sentry/browser (incl. Tracing, Replay) 78.55 kB +0.02% +15 B 🔺
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags 68.25 kB +0.02% +10 B 🔺
@sentry/browser (incl. Tracing, Replay with Canvas) 83.21 kB +0.02% +15 B 🔺
@sentry/browser (incl. Tracing, Replay, Feedback) 95.41 kB +0.02% +14 B 🔺
@sentry/browser (incl. Feedback) 40.87 kB - -
@sentry/browser (incl. sendFeedback) 28.81 kB - -
@sentry/browser (incl. FeedbackAsync) 33.75 kB - -
@sentry/react 25.88 kB - -
@sentry/react (incl. Tracing) 42.17 kB +0.03% +9 B 🔺
@sentry/vue 28.64 kB - -
@sentry/vue (incl. Tracing) 41.99 kB +0.03% +11 B 🔺
@sentry/svelte 24.19 kB - -
CDN Bundle 25.74 kB - -
CDN Bundle (incl. Tracing) 40.06 kB +0.03% +9 B 🔺
CDN Bundle (incl. Tracing, Replay) 76.28 kB +0.02% +12 B 🔺
CDN Bundle (incl. Tracing, Replay, Feedback) 81.79 kB +0.02% +12 B 🔺
CDN Bundle - uncompressed 75.18 kB - -
CDN Bundle (incl. Tracing) - uncompressed 118.53 kB +0.02% +23 B 🔺
CDN Bundle (incl. Tracing, Replay) - uncompressed 233.63 kB +0.01% +23 B 🔺
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed 246.39 kB +0.01% +23 B 🔺
@sentry/nextjs (client) 44.16 kB +0.02% +7 B 🔺
@sentry/sveltekit (client) 40.6 kB +0.02% +7 B 🔺
@sentry/node-core 49.88 kB +0.04% +15 B 🔺
@sentry/node 151.14 kB -0.02% -16 B 🔽
@sentry/node - without tracing 91.76 kB +0.02% +12 B 🔺
@sentry/aws-serverless 105.2 kB +0.01% +7 B 🔺

View base workflow run

Copy link
Contributor

github-actions bot commented Sep 17, 2025

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.
⚠️ Warning: Base artifact is not the latest one, because the latest workflow run is not done yet. This may lead to incorrect results. Try to re-run all tests to get up to date results.

Scenario Requests/s % of Baseline Prev. Requests/s Change %
GET Baseline 8,739 - 9,114 -4%
GET With Sentry 1,311 15% 1,436 -9%
GET With Sentry (error only) 5,862 67% 6,318 -7%
POST Baseline 1,183 - 1,222 -3%
POST With Sentry 498 42% 536 -7%
POST With Sentry (error only) 1,030 87% 1,069 -4%
MYSQL Baseline 3,327 - 3,407 -2%
MYSQL With Sentry 370 11% 539 -31%
MYSQL With Sentry (error only) 2,590 78% 2,785 -7%

View base workflow run

mydea added a commit that referenced this pull request Sep 17, 2025
Noticed while working on
#17679, we
incorrectly changed the `this` context here (which is also why TS
complained).

I rewrote this to a proxy which should keep the context properly.
@mydea mydea force-pushed the fn/handleCallbackErrors branch from 0a7f229 to 1222e4e Compare September 17, 2025 12:36
mydea added a commit that referenced this pull request Sep 17, 2025
…nction` (#17680)

Noticed while working on
#17679, this was
actually incorrect here. `handleCallbackErrors` does not actually do
anything, you need to still call the function you want to call in the
error case!

Actually we can just drop this as this error is handled by some other
handler anyhow, I think, according to the tests!
@mydea mydea force-pushed the fn/handleCallbackErrors branch from 1222e4e to 0fbf6eb Compare September 17, 2025 12:38
cursor[bot]

This comment was marked as outdated.

I've seen a few places where we are wrapping things that could be sync or async, and we want to do something with the return value. By adding an onSuccess handler to `handelCallbackErrors` we can handle this more generically in the future.
revert and fix
vendor type in

better awaited??
@mydea mydea force-pushed the fn/handleCallbackErrors branch from 0fbf6eb to 0e61da1 Compare September 17, 2025 12:47
@mydea mydea merged commit a48cf89 into develop Sep 17, 2025
186 of 188 checks passed
@mydea mydea deleted the fn/handleCallbackErrors branch September 17, 2025 13:24
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