EMBR-5086 updates to redux instrumentation#22
Conversation
jpmunz
commented
Jan 15, 2025
jpmunz
commented
- adding outcomes
- switching attribute customization to a function
- adding a test for when there's another middleware setup
| const provider = createInstanceProvider(); | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/ban-ts-comment | ||
| // @ts-ignore |
There was a problem hiding this comment.
@facostaembrace I noticed a few of these @ts-ignore lines are they all needed? My IDE was also complaining about this typing from line 44: middleware(undefined, { debug: false })(store);
| const result = next(action); | ||
| return next => { | ||
| return a => { | ||
| const action = a as Action; |
There was a problem hiding this comment.
the only way I found to get around the typescript error was to remove the typing from the functions here and then try and cast the unknown value back to an Action, given the way Redux types its middleware I'm not sure that there would be a better way since next, action, are all hardcoded as "unknown":
interface Middleware<_DispatchExt = {}, // TODO: see if this can be used in type definition somehow (can't be removed, as is used to get final dispatch type)
S = any, D extends Dispatch = Dispatch> {
(api: MiddlewareAPI<D, S>): (next: (action: unknown) => unknown) => (action: unknown) => unknown;
}
| const { | ||
| debug, | ||
| name, | ||
| attributeTransform = defaultAttributeTransform, |
There was a problem hiding this comment.
is there a particular reason why we are switching to a function? if so, should we apply the same to the otel navigation instrumentation library?
There was a problem hiding this comment.
this gave more flexibility to overriding the behaviour when we use it in the Embrace SDK here: https://github.com/embrace-io/embrace-react-native-sdk/blob/c6861108b07fa129251dc8b426a740f8ebdf31da/packages/react-native-redux/src/index.ts#L40
e.g. our backend expects payload_size so I was able to transform the existing payload attribute rather than only being able to add a static value
for the navigation case we're able to get what we need with just a static attribute for now but ya it probably does make sense to accept a function there as well to allow more options in the future
|
it seems like the update of |
I had skipped updating the package lock before, let me update and see if it addresses things |
The various "Unit Tests" ones still fail with I'll take a look at the peer-api-check one as that failure seems related to the otel api version in the package |
jpmunz
merged commit 51a0f5a
into
facosta/instrumentation-react-native-redux
2 participants
