feat: perf event handlers #553
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
zzmp
requested review from
just-toby,
a team and
vm
and removed request for
just-toby and
a team
| throw parsedError.status | ||
| } | ||
| } | ||
| throw queryError |
There was a problem hiding this comment.
is there a way to indicate in perf.ts that these can throw certain types of errors? should the integrator be aware so they can handle them?
There was a problem hiding this comment.
Yes, we could extend Promise to have a typed catch method: https://github.com/reduxjs/redux-toolkit/blob/ad508a18d3a475fafecfaa77cf2c50da755c7e3e/packages/toolkit/src/query/core/buildMiddleware/types.ts#L99
However, this would not maintain the typing with async-await usage, so I don't think it's worth the added effort (and I don't know that we'd get typechecking without avoiding async-await).
There was a problem hiding this comment.
I think a good follow up would be for us to export an enum (or extend Error) and only throw these errors from integration hooks, but I don't think that that should be a part of this PR. It could be a future improvement.
There was a problem hiding this comment.
sounds good - i was imagining some generic "throws" descriptor on the function , not even typing it. maybe something worth calling out in the docs if we update them for the PerfEventHandlers
src/hooks/swap/useWrapCallback.tsx
Outdated
| } | ||
| }, [parsedAmountIn, wrappedNativeCurrencyContract, wrapType]) | ||
| const callback = usePerfEventHandler('onWrapSend', wrapCallback, parsedAmountIn && { amount: parsedAmountIn }) |
There was a problem hiding this comment.
{ amount: parsedAmountIn } is inputs to wrapCallback right? kinda hard to tell without comments in usePerfEventHandler and without labelling these inputs
There was a problem hiding this comment.
refactored to (hopefully) make this more clear. usePerfEventHandler is meant to mirror the arguments of the perf event handler, so I want it to be ordinal inputs (ie not an options object).
There was a problem hiding this comment.
this makes sense now thanks
| throw parsedError.status | ||
| } | ||
| } | ||
| throw queryError |
There was a problem hiding this comment.
sounds good - i was imagining some generic "throws" descriptor on the function , not even typing it. maybe something worth calling out in the docs if we update them for the PerfEventHandlers
|
🎉 This PR is included in version 2.48.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Adds new perf event handlers so that relevant events can be instrumented by integrators (eg to track performance metrics):
onSwapQuoteonTokenAllowanceonPermit2AllowanceonSwapSendonWrapSendDeprecates existing handlers whose functionality is shared by the new handlers:
onSwapApprove,onSubmitSwapClick.Perf event handlers should record the entire event, so they all follow a similar pattern and use a new helper hook:
usePerfEventHandler. All perf event hooks take two parameters:argsand anevent. Theeventis a promise that resolves (or rejects) when the instrumented event has completed.usePerfEventHandlerwrapseventin a resolved Promise, so that it is not executed until after the perf event handler has executed. This allows integrators to instrument the full event (eg even if it triggers a fetch).INFRA-144