fix(insights): only `init` if `version` allows it #5529

FabienMotte · 2023-03-06T15:19:37Z

Summary

Alternative to #5523

If init has already been called outside on search-insights, calling init in the insights middleware would clear the previous options and we want to avoid that behavior.

Result

init is not called when the default Insights middleware is used.
init is called with partial: true option to update options without clearing them when a user pass a custom middleware.
init is called when insightsInitParams are passed (even if it's the default Insights middleware).

codesandbox-ci · 2023-03-06T15:23:13Z

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

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

packages/instantsearch.js/src/middlewares/createInsightsMiddleware.ts

Haroenv

this should be good once those versions are released. Needs a test of course then

FabienMotte · 2023-03-09T10:52:01Z

@Haroenv Thanks! Should I close #5523 then?

Haroenv · 2023-03-09T10:58:39Z

Yes, I think #5523 is a dead end. We may need to do it later still if there's an option coming from the response, but that doesn't block this PR I think.

packages/instantsearch.js/src/middlewares/createInsightsMiddleware.ts

Haroenv

I'm not convinced we should return if we can't init (as it realistically should be optional or done by the user)

packages/instantsearch.js/src/middlewares/createInsightsMiddleware.ts

yarn.lock

tests/mocks/createInsightsClient.ts

# Conflicts: # packages/instantsearch.js/src/middlewares/createInsightsMiddleware.ts # tests/mocks/createInsightsClient.ts

tests/mocks/createInsightsClient.ts

Haroenv

👍 I think this is the way to go!

packages/instantsearch.js/src/middlewares/createInsightsMiddleware.ts

# Conflicts: # bundlesize.config.json

packages/instantsearch.js/src/middlewares/__tests__/createInsightsMiddleware.ts

packages/instantsearch.js/src/middlewares/createInsightsMiddleware.ts

dhayab

LGTM. I'm stealing the comment description of isModernInsightsClient() for my PR 😛!

FabienMotte · 2023-04-12T14:23:11Z

Thanks a lot @dhayab!

Co-authored-by: Dhaya <154633+dhayab@users.noreply.github.com>

Haroenv reviewed Mar 6, 2023

View reviewed changes

packages/instantsearch.js/src/middlewares/createInsightsMiddleware.ts Outdated Show resolved Hide resolved

Haroenv reviewed Mar 6, 2023

View reviewed changes

packages/instantsearch.js/src/middlewares/createInsightsMiddleware.ts Outdated Show resolved Hide resolved

FabienMotte requested review from Haroenv, sarahdayan and dhayab March 8, 2023 13:04

FabienMotte force-pushed the fix/insights-init-version branch from b345868 to a972714 Compare March 8, 2023 15:13

Haroenv force-pushed the feat/automatic-insights branch from b4e9504 to 7f415a8 Compare March 8, 2023 16:00

FabienMotte added 2 commits March 8, 2023 17:12

fix(insights): only init if version allows it

ed517dc

fix: check version in internal only

c5c1da4

FabienMotte force-pushed the fix/insights-init-version branch from a972714 to c5c1da4 Compare March 8, 2023 16:13

FabienMotte mentioned this pull request Mar 8, 2023

fix(insights): call init only if it has not been called before #5523

Closed

Haroenv reviewed Mar 9, 2023

View reviewed changes

Haroenv reviewed Mar 14, 2023

View reviewed changes

packages/instantsearch.js/src/middlewares/createInsightsMiddleware.ts Outdated Show resolved Hide resolved

Merge branch 'feat/automatic-insights' into fix/insights-init-version

4b622d1

Haroenv reviewed Mar 20, 2023

View reviewed changes

packages/instantsearch.js/src/middlewares/createInsightsMiddleware.ts Outdated Show resolved Hide resolved

packages/instantsearch.js/src/middlewares/createInsightsMiddleware.ts Outdated Show resolved Hide resolved

move version + flag to the right branch

cfa4384

dhayab marked this pull request as ready for review March 20, 2023 18:12

dhayab requested a review from Haroenv March 20, 2023 18:12

Haroenv requested changes Mar 21, 2023

View reviewed changes

packages/instantsearch.js/src/middlewares/createInsightsMiddleware.ts Outdated Show resolved Hide resolved

yarn.lock Outdated Show resolved Hide resolved

dhayab marked this pull request as draft March 21, 2023 09:18

fix tests

383ee5a

dhayab reviewed Mar 21, 2023

View reviewed changes

tests/mocks/createInsightsClient.ts Outdated Show resolved Hide resolved

dhayab mentioned this pull request Mar 21, 2023

fix(insights): send extra parameters only for applicable versions #5558

Merged

FabienMotte added 4 commits April 5, 2023 11:38

Merge branch 'feat/automatic-insights' into fix/insights-init-version

9044e66

# Conflicts: # packages/instantsearch.js/src/middlewares/createInsightsMiddleware.ts # tests/mocks/createInsightsClient.ts

remove useless type

ea878ee

tests: fix insights client mock type

71b70de

only init if user provided the insights middleware

30a281a

FabienMotte added 2 commits April 5, 2023 17:17

chore: update bundlesize config

37d93a8

test: improve

c4f8047

FabienMotte commented Apr 6, 2023

View reviewed changes

tests/mocks/createInsightsClient.ts Outdated Show resolved Hide resolved

FabienMotte marked this pull request as ready for review April 6, 2023 10:29

FabienMotte requested review from Haroenv and dhayab April 6, 2023 10:30

Haroenv mentioned this pull request Apr 6, 2023

feat(insights): add "insights" option for automatic events #5490

Merged

Haroenv requested changes Apr 6, 2023

View reviewed changes

packages/instantsearch.js/src/middlewares/createInsightsMiddleware.ts Outdated Show resolved Hide resolved

FabienMotte added 3 commits April 6, 2023 14:23

test: use castToJestMock util

c879921

fix: init if insightsInitParams are passed

fec929d

Merge branch 'feat/automatic-insights' into fix/insights-init-version

52f54bf

# Conflicts: # bundlesize.config.json

Haroenv approved these changes Apr 6, 2023

View reviewed changes

sarahdayan reviewed Apr 6, 2023

View reviewed changes

packages/instantsearch.js/src/middlewares/__tests__/createInsightsMiddleware.ts Show resolved Hide resolved

fix: init if optional init method not supported by insights client

7c1623b

Haroenv reviewed Apr 7, 2023

View reviewed changes

packages/instantsearch.js/src/middlewares/createInsightsMiddleware.ts Outdated Show resolved Hide resolved

refactor: rename function and add jsdoc comment

fd0b44e

FabienMotte requested a review from sarahdayan April 7, 2023 16:30

Haroenv approved these changes Apr 11, 2023

View reviewed changes

dhayab approved these changes Apr 12, 2023

View reviewed changes

FabienMotte merged commit e489236 into feat/automatic-insights Apr 12, 2023

FabienMotte deleted the fix/insights-init-version branch April 12, 2023 14:23

Haroenv pushed a commit that referenced this pull request Apr 18, 2023

fix(insights): only init if version allows it (#5529)

fa98e21

Co-authored-by: Dhaya <154633+dhayab@users.noreply.github.com>

Haroenv pushed a commit that referenced this pull request Apr 24, 2023

fix(insights): only init if version allows it (#5529)

8537f8f

Co-authored-by: Dhaya <154633+dhayab@users.noreply.github.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(insights): only `init` if `version` allows it #5529

fix(insights): only `init` if `version` allows it #5529

FabienMotte commented Mar 6, 2023 •

edited

Loading

codesandbox-ci bot commented Mar 6, 2023 •

edited

Loading

Haroenv left a comment •

edited

Loading

FabienMotte commented Mar 9, 2023

Haroenv commented Mar 9, 2023

Haroenv left a comment

Haroenv left a comment

dhayab left a comment

FabienMotte commented Apr 12, 2023

fix(insights): only init if version allows it #5529

fix(insights): only init if version allows it #5529

Conversation

FabienMotte commented Mar 6, 2023 • edited Loading

codesandbox-ci bot commented Mar 6, 2023 • edited Loading

Haroenv left a comment • edited Loading

Choose a reason for hiding this comment

FabienMotte commented Mar 9, 2023

Haroenv commented Mar 9, 2023

Haroenv left a comment

Choose a reason for hiding this comment

Haroenv left a comment

Choose a reason for hiding this comment

dhayab left a comment

Choose a reason for hiding this comment

FabienMotte commented Apr 12, 2023

fix(insights): only `init` if `version` allows it #5529

fix(insights): only `init` if `version` allows it #5529

FabienMotte commented Mar 6, 2023 •

edited

Loading

codesandbox-ci bot commented Mar 6, 2023 •

edited

Loading

Haroenv left a comment •

edited

Loading