-
Notifications
You must be signed in to change notification settings - Fork 332
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: allow extra arguments in Insights API #1210
Conversation
@@ -6,6 +6,9 @@ export type AutocompleteInsightsApi = ReturnType< | |||
typeof createSearchInsightsApi | |||
>; | |||
|
|||
export type WithArbitraryParams<TParams extends Record<string, unknown>> = |
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.
It's technically an all-purpose utility type, but since it's only being used here for now, it seemed like an unnecessary hassle to move it to shared
.
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 0b43fc9:
|
> | ||
) { | ||
if (params.length > 0) { | ||
sendToInsights( | ||
'clickedObjectIDsAfterSearch', | ||
mapToInsightsParamsApi(params), | ||
mapToInsightsParamsApi< |
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.
The way mapToInsightsParamsApi
is typed is what's causing the typing issues I was encountering. When I inline the implementation of mapToInsightsParamsApi
directly, the issue disappears.
I'm not sure what's going on exactly in how TypeScript interprets it all, but when unioning a strict record with an arbitrary record, the generic type TInsightsParamsType
loses all the mandatory properties and the return type only contains objectIDs
.
I'm doubtful we can fix this at the mapToInsightsParamsApi
level, so the pragmatic option seems to cast.
Lmk if you have a working option that works better.
Summary
This types and tests the exposed Insights API to allow passing extra arguments in the payload.
This was already possible, but would have resulted in typing violations for TypeScript users.
FX-2652