fix(insights): do not throw when sending event right after creating insights middleware #4575

eunjae-lee · 2020-11-09T16:01:48Z

Summary

There is a bug where an error is thrown when sending events right after creating insights middleware.

Details

It happens only when:

The UMD version of search-insights is used.
The search-insights is not loaded yet at the time of creation of insights middleware.

In insights middleware, the way it intializes insightsClient is asynchronous. If it happens too late (normally it happens when the library itself hasn't loaded yet), sending event will throw an error saying "the insightsClient is not intialized yet".

So, in this PR, we change to initialize insightsClient synchronously without checking if it was already initialized before.

A behavioral change

Before this PR, userToken that was set before creating insights middleware was ignored. However since this PR, that has to change, because although user has a code like the following:

aa('init', { ... });
aa('setUserToken', 'token-a');

search.use(createInsightsMiddleware({ ... }));

insights middleware calls aa('init', ...) anyway. It means aa(init', ...) will override the token with an anonymous token. Of course, we can update search-insights not to override already given token with an anonymous one, but there is no guarantee that user upgrades search-insights correctly. So we cannot rely on it.

Since insights middleware initializes insightsClient, we guide not to call aa('init') by themselves, but we cannot stop it. And if they set userToken like above, we shouldn't lose the token (although before this PR, we used to ignore the token before creating the middleware).

This behavioral change can be considered as a breaking change, but we can also consider it as a "fix" because it is correct to prefer a userToken that was explicitly set by user over an anonymous token.

Alternatives

There are a couple of other options requiring changes in search-insights side, but it's risky to have a change in InstantSearch that depends on a change in search-insights because we cannot guarantee users will upgrade them both.

Future task

This doesn't concern another issue we've recently discussed about, which is to provide an option to disable cookie at all. It will be dealt separately.

Result

When UMD search-insights is not loaded yet, it doesn't throw when sending events. It stores it in the queue and send it later correctly.
If user sets a token before creating insights middleware, it takes it into account. (It used to not.)

…ht after creating the middleware

codesandbox-ci · 2020-11-09T16:02:54Z

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 b3e0247:

Sandbox	Source
InstantSearch.js	Configuration

eunjae-lee · 2020-11-09T16:06:10Z

src/middlewares/__tests__/createInsightsMiddleware.ts

@@ -36,6 +36,30 @@ describe('insights', () => {
    };
  };

+  const createUmdTestEnvironment = () => {


This has been moved here from other describe scope with no change.

eunjae-lee · 2020-11-09T16:07:08Z

src/middlewares/__tests__/createInsightsMiddleware.ts

@@ -236,6 +251,7 @@ aa('setUserToken', 'your-user-token');`);
          insightsClient,
        })({ instantSearchInstance });
        middleware.subscribe();
+        libraryLoadedAndProcessQueue();


This was missing. We need this because the library will eventually be loaded.

eunjae-lee · 2020-11-09T16:50:42Z

src/middlewares/createInsightsMiddleware.ts

        }

-        if (Array.isArray((insightsClient as any).queue)) {


This part has been move to the line 44. And there it doesn't call setUserTokenToSearch(). It instead sets a variable there, and later on, at the line 95, it reads the variable and sets it.

francoischalifour · 2020-11-10T12:37:44Z

src/middlewares/createInsightsMiddleware.ts

+      // At this point, even though `search-insights` is not loaded yet,
+      // we still want to read the token from the queue.
+      // Otherwise, the first search call will be fired without the token.
+      (insightsClient as any).queue.forEach(([method, firstArgument]) => {


Why using forEach if we override the variable every time? Using find would make more sense:

find(insightsClient.queue.slice().reverse(), /* ... */)

slice to not mutate the queue

reverse to find the latest occurrence

francoischalifour · 2020-11-10T12:40:41Z

src/middlewares/createInsightsMiddleware.ts

@@ -38,26 +38,35 @@ export const createInsightsMiddleware: CreateInsightsMiddleware = props => {
    _insightsClient === null ? (noop as InsightsClient) : _insightsClient;


If we type it correctly here, we shouldn't have to cast insightsClient every time.

Do you mean regarding (insightsClient as any).queue right?
5ddedee

francoischalifour · 2020-11-10T12:46:05Z

src/middlewares/createInsightsMiddleware.ts

+          setUserTokenToSearch(anonymousUserToken);
+        }
+
+        if (queuedUserToken) {


Why can this condition and the one below be satisfied at the same time? It makes the flow harder to understand.

If that what it means, I'd suggest changing to this:

// We consider the `userToken` coming from a `init` call to have a higher // importance than the one coming from the queue. if (userTokenBeforeInit) { insightsClient('setUserToken', userTokenBeforeInit); } else if (queuedUserToken) { insightsClient('setUserToken', queuedUserToken); }

francoischalifour · 2020-11-10T12:47:37Z

test/mock/createInsightsClient.ts

@@ -26,13 +26,23 @@ export function createAlgoliaAnalytics() {
      callback(values._userToken);
    }
  };
+  const sendEvent = () => {


Why do we implement another whole logic in its mock?

Good point. I thought about it again, and I think the new test is better.
0add3dc

We probably shouldn't even mock any logic, but just spy for methods called. That's coming from before so it shouldn't block that PR though.

At some point, I began to wonder if I just should've used the real search-insights client for the tests, because there are some implementations on the search-insights side that needs to be tested with the insights middleware.

francoischalifour · 2020-11-12T16:25:56Z

src/middlewares/__tests__/createInsightsMiddleware.ts

-// or
-aa('setUserToken', 'your-user-token');`);
+      // It tries to send an event.
+      instantSearchInstance.sendEventToInsights({


We keep testing internals here, that's fragile. Again, that's how it was before these changes.

Although the method is public, in this context, we're indeed testing the internals. I'll keep it that way for now.

eunjae-lee added 2 commits November 9, 2020 14:08

test: test if insightsClient does not throw when an event is sent rig…

97244a5

…ht after creating the middleware

fix: restore user token which was set before creating the middleware

9effefd

eunjae-lee commented Nov 9, 2020

View reviewed changes

eunjae-lee changed the title ~~fix: restore user token which was set before creating the middleware~~ fix(insights): do not throw when sending event right after creating insights middleware Nov 9, 2020

test: make it clearer

1d3c977

eunjae-lee commented Nov 9, 2020

View reviewed changes

eunjae-lee marked this pull request as ready for review November 9, 2020 16:55

eunjae-lee requested review from francoischalifour and Haroenv November 9, 2020 16:56

francoischalifour reviewed Nov 10, 2020

View reviewed changes

eunjae-lee and others added 5 commits November 12, 2020 10:43

use find instead of loop

fa7ea26

add queue to InsightsClient type

5ddedee

clean up flow

8b10f89

change the way to test viewedObjectIDs

0add3dc

Merge branch 'master' into fix/insights-init

b3e0247

eunjae-lee requested a review from francoischalifour November 12, 2020 10:17

francoischalifour approved these changes Nov 12, 2020

View reviewed changes

eunjae-lee merged commit d963f8d into master Nov 12, 2020

eunjae-lee deleted the fix/insights-init branch November 12, 2020 17:15

francoischalifour mentioned this pull request Nov 17, 2020

fix(insights): use internal find util method #4580

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(insights): do not throw when sending event right after creating insights middleware #4575

fix(insights): do not throw when sending event right after creating insights middleware #4575

eunjae-lee commented Nov 9, 2020 •

edited

Loading

codesandbox-ci bot commented Nov 9, 2020 •

edited

Loading

eunjae-lee Nov 9, 2020

eunjae-lee Nov 9, 2020

eunjae-lee Nov 9, 2020

francoischalifour Nov 10, 2020

eunjae-lee Nov 12, 2020

francoischalifour Nov 10, 2020

eunjae-lee Nov 12, 2020

francoischalifour Nov 10, 2020

eunjae-lee Nov 12, 2020

francoischalifour Nov 10, 2020

eunjae-lee Nov 12, 2020

francoischalifour Nov 12, 2020

eunjae-lee Nov 12, 2020

francoischalifour Nov 12, 2020

eunjae-lee Nov 12, 2020

		@@ -38,26 +38,35 @@ export const createInsightsMiddleware: CreateInsightsMiddleware = props => {
		_insightsClient === null ? (noop as InsightsClient) : _insightsClient;

fix(insights): do not throw when sending event right after creating insights middleware #4575

fix(insights): do not throw when sending event right after creating insights middleware #4575

Conversation

eunjae-lee commented Nov 9, 2020 • edited Loading

Summary

Details

A behavioral change

Alternatives

Future task

Result

codesandbox-ci bot commented Nov 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eunjae-lee commented Nov 9, 2020 •

edited

Loading

codesandbox-ci bot commented Nov 9, 2020 •

edited

Loading