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(insights): accept initParams for insightsClient #4608

Merged
merged 2 commits into from
Jan 11, 2021

Conversation

eunjae-lee
Copy link
Contributor

@eunjae-lee eunjae-lee commented Dec 15, 2020

Summary

This PR accepts new initParams at insights middleware for initializing insightsClient.

const middleware = createInsightsMiddleware({
  insightsClient,
  insightsInitParams: {
    useCookie: false,
  },
})

It can be used to disable cookie like above. (The useCookie option is being implemented at the moment in algolia/search-insights.js#236)

An extra thought about useCookie: false

https://codesandbox.io/s/instantsearchjs-forked-5wu8h?file=/src/app.js:976-1197

If user passes useCookie: false, the insightsClient won't have any token at the time of initialization (because it will skip generating an anonymous user token).

When the insights middleware will try to send events, insightsClient will throw, saying userToken is missing. However in that case, it doesn't make sense to send events when userToken is not specified. So they will have to postpone adding insights middleware.

search.start();

const insights = createInsightsMiddleware({
  insightsClient,
  insightsInitParams: {
    useCookie: false,
  },
});

setTimeout(() => {
  // Whenever a user token is ready,
  // the insights middleware can be used.
  // It doesn't necessarily have to be used from the beginning.
  insightsClient('setUserToken', 'user-token-1');
  search.use(insights);
}, 3000);

@codesandbox-ci
Copy link

codesandbox-ci bot commented Dec 15, 2020

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 8bbb98e:

Sandbox Source
InstantSearch.js Configuration
InstantSearch.js (forked) PR

@eunjae-lee eunjae-lee requested a review from a team December 15, 2020 16:18
@ghost ghost requested review from Haroenv and shortcuts and removed request for a team December 15, 2020 16:18
@Haroenv
Copy link
Contributor

Haroenv commented Dec 17, 2020

what if the events get queued until the user token is given, so they don't get forgotten (like it works with init & umd)?

@eunjae-lee
Copy link
Contributor Author

what if the events get queued until the user token is given, so they don't get forgotten (like it works with init & umd)?

but search-insights throws error immediately when trying to send event without token. If we want it, we should change from search-insights. Otherwise, we will end up having two different experiences.

And if we just queue it and wait for user token, it might be problematic if they forgot to set user token and the events will be missing with no error or warning.

@Haroenv
Copy link
Contributor

Haroenv commented Dec 24, 2020

regardless of what I'm suggesting next, I feel like this PR makes sense, however I think we should rethink useCookie; instead we should add userToken to init params.

If you add a userToken to init, there's no need for the anonymous token, and thus no use for the cookie either. This doesn't solve the "we want anonymous token but no cookie" use case, but neither does useCookie: false today.

You could pass userToken: magicValueWhichMeansGenerateAnonymous (maybe that value is false, null or something like that, indicating it's unset)

@eunjae-lee
Copy link
Contributor Author

@Haroenv That is actually a good point.

With userToken added to init params, this whole scenario will make sense. I'll prepare a PR for that in search-insights.

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