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(createInsightsMiddleware): always pass Algolia credentials locally #5554

Merged
merged 2 commits into from
Mar 20, 2023

Conversation

sarahdayan
Copy link
Member

@sarahdayan sarahdayan commented Mar 17, 2023

This attaches the Algolia credentials to each event we send, via headers.

The goal is to ensure events are always sent to the right app even when the Insights client is shared—for example, when using InstantSearch together with Autocomplete.

FX-2278

@codesandbox-ci
Copy link

codesandbox-ci bot commented Mar 17, 2023

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 0b73dfd:

Sandbox Source
InstantSearch.js Configuration
react-instantsearch-app Configuration
example-react-instantsearch-hooks-default-theme Configuration
example-vue-instantsearch-default-theme Configuration

@sarahdayan sarahdayan force-pushed the feat/local-insights-credentials branch from 1299efc to 71f997d Compare March 17, 2023 14:32
instantSearchInstance.sendEventToInsights = (event: InsightsEvent) => {
if (onEvent) {
onEvent(event, _insightsClient as TInsightsClient);
Copy link
Member Author

Choose a reason for hiding this comment

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

I believe passing _insightsClient here was a bug, as this wouldn't point to the right function in an auto-pulled scenario.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah yes, with auto-pulling that is wrong, but it used to be the user-provided one on purpose in case someone passes null, the insightsClient would be null and not noop, but that's not very different

Copy link
Member Author

@sarahdayan sarahdayan Mar 17, 2023

Choose a reason for hiding this comment

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

Do you think it should stay nullable (e.g., when opting-out of auto-pull)?

@sarahdayan sarahdayan force-pushed the feat/local-insights-credentials branch from 71f997d to 102d61f Compare March 17, 2023 15:00
@sarahdayan sarahdayan marked this pull request as ready for review March 17, 2023 15:01
@sarahdayan sarahdayan force-pushed the feat/local-insights-credentials branch from 102d61f to 0b73dfd Compare March 17, 2023 15:15
@sarahdayan sarahdayan requested review from Haroenv and dhayab March 17, 2023 15:15
Copy link
Contributor

@Haroenv Haroenv left a comment

Choose a reason for hiding this comment

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

👍

Comment on lines +228 to +229
// @ts-ignore
const insightsClientWithLocalCredentials = (method, payload) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// @ts-ignore
const insightsClientWithLocalCredentials = (method, payload) => {
const insightsClientWithLocalCredentials: InsightsClient = (method, payload) => {

does that work without angering TS?

Copy link
Member Author

@sarahdayan sarahdayan Mar 17, 2023

Choose a reason for hiding this comment

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

Not yet, we need to release the new version of the Insights client so that the types are up to date.

I can do it in a different PR if we don't release it before we're ready to merge this one.

Copy link
Contributor

@Haroenv Haroenv left a comment

Choose a reason for hiding this comment

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

looks good like this

@sarahdayan sarahdayan merged commit 35db0c1 into feat/automatic-insights Mar 20, 2023
@sarahdayan sarahdayan deleted the feat/local-insights-credentials branch March 20, 2023 13:57
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.

3 participants