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): handle multiple setUserToken call before search.start() #4897

Merged
merged 5 commits into from
Sep 7, 2021

Conversation

eunjae-lee
Copy link
Contributor

Summary

In #4849, we introduced a regression that insights middleware throws when setUserToken is called twice before search.start().

In the revised implementation of the insights middleware either adds a configure widget with userToken or calls refine function of renderState. However when instantSearchInstance is not started yet, even though configure widget is added, there is no renderState. That's why it's failing.

@eunjae-lee eunjae-lee changed the title fix(insights): apply the last userToken before search.start() fix(insights): handle multiple setUserToken call before search.start() Sep 6, 2021
@codesandbox-ci
Copy link

codesandbox-ci bot commented Sep 6, 2021

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 767a506:

Sandbox Source
InstantSearch.js Configuration

@Haroenv
Copy link
Contributor

Haroenv commented Sep 6, 2021

please also make the renderState optional so typescript will catch this in the future :)

@eunjae-lee
Copy link
Contributor Author

// implementation #1
const setUserTokenToSearch = (userToken?: string) => {
  const refineFromRenderState =
    instantSearchInstance.renderState?.[instantSearchInstance.indexName]
      ?.configure?.refine;

  if (configureUserToken && refineFromRenderState) {
    refineFromRenderState({ userToken });
  } else {
    if (configureUserToken) {
      instantSearchInstance.removeWidgets([configureUserToken]);
    }
    configureUserToken = createWidget({
      searchParameters: { userToken },
    });
    instantSearchInstance.addWidgets([configureUserToken]);
  }
};

// implementation #2
const setUserTokenToSearch = (userToken?: string) => {
  if (configureUserToken) {
    instantSearchInstance.removeWidgets([configureUserToken]);
  }
  configureUserToken = createWidget({
    searchParameters: { userToken },
  });
  instantSearchInstance.addWidgets([configureUserToken]);
};

In #4849, we tried to avoid "adding/removing widget" situation. That's why I used refine function from renderState once the widget is added. Now that we know renderState isn't there if instantSearchInstance is not started, it's kind of unavoidable. Does implementation #2 make sense? Or do we have another idea? cc @Haroenv

@Haroenv
Copy link
Contributor

Haroenv commented Sep 6, 2021

Another option to continue using renderState might be to somehow get access to the render state of the widget without waiting for InstantSearch to have it available. I'm not really sure, as it will expect the arguments given to init at least, but maybe there's an option?

@eunjae-lee
Copy link
Contributor Author

Another option to continue using renderState might be to somehow get access to the render state of the widget without waiting for InstantSearch to have it available. I'm not really sure, as it will expect the arguments given to init at least, but maybe there's an option?

Yeah that's a path. I'll look if it's possible tomorrow.

@eunjae-lee
Copy link
Contributor Author

@Haroenv I hoped that configureWidget.getWidgetRenderState(...) would work, but it takes helper as an argument. At subscribe() of middleware, we don't have a helper yet.

Since we aim to be able to attach userToken from the very first search call, I cannot think of other option.

Copy link
Contributor

@Haroenv Haroenv left a comment

Choose a reason for hiding this comment

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

let's keep it simple and fix this issue quickly, I think your solution that's not commented out is the way to go

@eunjae-lee eunjae-lee marked this pull request as ready for review September 7, 2021 08:33
@eunjae-lee eunjae-lee enabled auto-merge (squash) September 7, 2021 08:34
Copy link
Contributor

@Haroenv Haroenv left a comment

Choose a reason for hiding this comment

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

removing and adding a widget is heavier than refining after search, but before it's the only option. LGTM

@eunjae-lee
Copy link
Contributor Author

removing and adding a widget is heavier than refining after search, but before it's the only option. LGTM

yeah indeed. We could conditionally do differently between before and after search, but keeping it simple for now will help us refactor or improve it later.

@eunjae-lee eunjae-lee merged commit 51a6f2b into master Sep 7, 2021
@eunjae-lee eunjae-lee deleted the fix/insights-undefined-renderstate branch September 7, 2021 08:40
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.

2 participants