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

fix(insights): ensure the same token is used when rendered multiple times server side #6456

Merged
merged 10 commits into from
Dec 9, 2024

Conversation

Haroenv
Copy link
Contributor

@Haroenv Haroenv commented Dec 3, 2024

  • in insights middleware: ensure we read from the _initialResults (as insights starts before index is init'ed and the parameters aren't in the index, but in the main index)
  • in getInitialResults: add the parameters to every index explicitly (it's not ui state so was missing otherwise)

other changes in this PR:

related to CR-6992

…es server side

in insights middleware: ensure we read from the _initialResults (as insights starts before index is init'ed
in getInitialResults: add the parameters to every index explicitly (it's not ui state so was missing otherwise)

other changes in this PR:
- something in the dependencies to prevent babel/babel#16689
- use a cache in the server side react examples
- type `userToken` in SearchParameters
@Haroenv Haroenv requested review from a team, sarahdayan and shaejaz and removed request for a team December 3, 2024 08:51
Copy link

codesandbox-ci bot commented Dec 3, 2024

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 59c9e64:

Sandbox Source
example-instantsearch-getting-started Configuration
example-react-instantsearch-getting-started Configuration
example-react-instantsearch-next-app-dir-example Configuration
example-react-instantsearch-next-routing-example Configuration
example-vue-instantsearch-getting-started Configuration

@Haroenv Haroenv changed the title fix(insights): ensure _same_ token is used when rendered multiple times server side fix(insights): ensure the same token is used when rendered multiple times server side Dec 3, 2024
@Haroenv Haroenv marked this pull request as draft December 3, 2024 08:56
@Haroenv Haroenv marked this pull request as ready for review December 3, 2024 10:50
@Haroenv Haroenv requested a review from shaejaz December 3, 2024 17:46
@shaejaz
Copy link
Contributor

shaejaz commented Dec 3, 2024

So does this mean the cache always needs to be manually cleared by the user on the server side? Maybe comment the reason for it in the examples? Or if you feel this would be very common, perhaps move it to instantsearch and have an optional opt out via a flag?

@Haroenv
Copy link
Contributor Author

Haroenv commented Dec 4, 2024

The client cache in the examples is a bit of a stop-gap solution, I'm exploring some options for a follow-up:

  • the client automatically has a "one response cache" or a cache with a low TTL
  • we "cache" (prevent the request) if the parameters are the same in InstantSearch (but can't find a good place for it)

With this temporary solution at least we have something to point users towards to avoid the secondary request with dynamic widgets.

Copy link
Contributor

@shaejaz shaejaz left a comment

Choose a reason for hiding this comment

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

Ah makes sense. lgtm.

@Haroenv Haroenv added the 🚨 DO NOT MERGE for a pull request that is ready to review, but should not be merged yet label Dec 4, 2024
@dhayab dhayab removed the 🚨 DO NOT MERGE for a pull request that is ready to review, but should not be merged yet label Dec 9, 2024
@Haroenv Haroenv enabled auto-merge (squash) December 9, 2024 09:29
@Haroenv Haroenv merged commit c3a1c70 into master Dec 9, 2024
14 checks passed
@Haroenv Haroenv deleted the fix/insights-same-token branch December 9, 2024 09:35
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