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): automatically add insights middleware #5488

Merged
merged 5 commits into from
Feb 23, 2023

Conversation

Haroenv
Copy link
Contributor

@Haroenv Haroenv commented Feb 20, 2023

Summary

Automatically add the insights middleware

Result

FX-2248

  • add the insights middleware automatically
  • identify internal vs. user-provided middleware
  • make sure adding or removing the automatic insights middleware doesn't cause a search
  • when a user adds a different insights middleware, it takes precedence
  • update a few tests that now have clickAnalytics: true

@codesandbox-ci
Copy link

codesandbox-ci bot commented Feb 20, 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 3d7aa28:

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

@Haroenv Haroenv marked this pull request as ready for review February 20, 2023 14:52
@Haroenv Haroenv requested review from a team, sarahdayan and aymeric-giraudet and removed request for a team February 20, 2023 14:52
@aymeric-giraudet
Copy link
Member

Nice ! 🙌

@Haroenv Haroenv force-pushed the feat/automatic-insights-allow-auto branch from 4685922 to de9bf32 Compare February 21, 2023 13:29
@Haroenv Haroenv force-pushed the feat/automatic-insights-middleware branch from 5fc27cb to 5b35140 Compare February 21, 2023 13:31
Base automatically changed from feat/automatic-insights-allow-auto to feat/automatic-insights February 21, 2023 14:17
@Haroenv Haroenv force-pushed the feat/automatic-insights-middleware branch from 5b35140 to 3d7aa28 Compare February 21, 2023 14:25
Comment on lines +306 to +308
// This is the default middleware,
// any user-provided middleware will be added later and override this one.
this.use(createInsightsMiddleware({ $$internal: true }));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

something still needs to be fixed here, the script is always loaded from the internal middleware, even if users add a different middleware later (even right after). Not too sure how to fix that yet

Copy link
Contributor Author

Choose a reason for hiding this comment

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

solved through #5493

@algolia algolia deleted a comment from codesandbox-ci bot Feb 22, 2023
@Haroenv Haroenv requested a review from dhayab February 23, 2023 08:47
Comment on lines +156 to +159
function normalizeState(state: PlainSearchParameters) {
const { clickAnalytics, userToken, ...rest } = state || {};
return rest;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

if infiniteHits is added before start, it's reading from the cache before the insights middleware is started, and thus it doesn't have clickAnalytics + userToken yet

Copy link
Contributor Author

Choose a reason for hiding this comment

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

to solve this, we could change some things about the insights middleware to make sure we have the search parameters right away, even before init, but that's currently not possible yet (helper is null)

if (isMetadataEnabled()) {
this.use(createMetadataMiddleware());
this.use(createMetadataMiddleware({ $$internal: true }));
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: this is technically always internal, it could be set statically in createMetadataMiddleware().

@Haroenv Haroenv merged commit c785c86 into feat/automatic-insights Feb 23, 2023
@Haroenv Haroenv deleted the feat/automatic-insights-middleware branch February 23, 2023 10:56
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