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

Batch actions and the new "shouldClearFilters" option #391

Merged
merged 21 commits into from
Sep 26, 2019

Conversation

JasonStoltz
Copy link
Member

@JasonStoltz JasonStoltz commented Sep 20, 2019

Description

In its current state, Search UI has a problem. Actions trigger API calls to the Search API. When there are in-flight calls to the Search API, Search UI will block any new actions.

That means, if a user were to call multiple actions in a row like this the following, that only the first action would be applied.

addFilter("states", "Alaska", "any");
addFilter("states", "California", "any");
setSearchTerm("park");

One way to fix this, would be to simply unblock subsequent actions from making API calls. However, you would then encounter an issue where the API is making excessive API calls. If you could imagine in the example above, that would result in 3 separate API calls.

In order to cleanly avoid this, I've added the ability for actions to be automatically batched.

An action can be thought of in 2 parts:

  1. A state update
  2. A side-effect

The primary side-effect of most actions is to execute a new query. All I've done is debounced that side effect, so only after the user is done calling a series of actions, does the side effect get executed.

Why would someone need to do this?

We currently don't have any sort of bulk API for applying various filters. Meaning, if you had a reason to apply 2 filters at once, there is no single action to do that. You must make two calls to the addFilter action.

addFilter("states", "Alaska", "any");
addFilter("world_heritage_site", "true");

Another case, is a search experience with a submit button, like an "Advanced Search" screen. These experiences typically are not updated live as you are making filter selections. Instead, you enter all of your selections and press a submit button to apply them at once.

In the future, we should have a better solution for this. But for now, that could be accomplished by simply hooking the "Submit" button up to a handler that calls all of the appropriate actions to apply the user's selections.

The case of "setSearchTerm" and "shouldClearFilters"

Currently, "setSearchTerm" clears out existing filters any time that it is executed. This is usually, but not always, desirable. Especially if you think about the case I mentioned above, where you are implementing an "Advanced Search" feature. Calling items in this order actually ends up clearing the filters that you just added when you call "setSearchTerm":

addFilter("states", "Alaska", "any");
addFilter("states", "California", "any");
setSearchTerm("park");

I added the ability to work around this with a "shouldClearFilters" flag:

addFilter("states", "Alaska", "any");
addFilter("states", "California", "any");
setSearchTerm("park", { shouldClearFilters: false });

The case of "setSearchTerm" and "debounce"

I had to think about what the implications of debouncing both the search side effect and also allowing users to debounce searches independently when using "setSearchTerm". For example:

setSearchTerm("park", { refresh: true, debounce: 1000 });
addFilter("states", "Alaska", "any");
addFilter("states", "California", "any");

What happened in this case, is that 2 queries were being executed.

  1. The initial query which applied the search term "park" as well as the 2 filters on "states".
  2. The second query, on "park", with NO filters applied. This is because the "setSearchTerm" side-effect was deferred by 1000 ms and ended up executing after the initial query.

To overcome this, I added logic that cancels any outstanding side-effects, as they should be considered stale and no longer valid to run if a new side-effect is applied.

Want to test this locally?

Add the following button to App.js in the sandbox example:

<button
  onClick={() => {
    addFilter("states", "Alaska", "any");
    setSearchTerm("park", { shouldClearFilters: false });
    addFilter("states", "California", "any");
  }}
>
  Click here
</button>

Clicking that button BEFORE my changes will only apply the California filter to results.

AFTER my changes, that will apply all 3 of those filters.

List of changes

  • Added batch actions
  • Added a "shouldClearFilters" flag to setSearchTerm
  • Added side-effect cancelling in our actions
  • Added the "shouldClearFilters" option to the SearchBox component as well.

Associated Github Issues

#65

Debouncing a setSearchTerm request causes strange behavior, because
it ends up making the original API request after the subsequent
actions. This has some odd side effects, like clearing out the filters
that were just set.

In order to work around this short term, I've added a "clearFilters"
flag which can be added to the setSearchTerm action.
Added the clearFilter option to the SearchBox, and updated
documentation.
@JasonStoltz JasonStoltz marked this pull request as ready for review September 20, 2019 21:00
@JasonStoltz JasonStoltz changed the title Batch actions Batch actions and the new "clearFilters" option Sep 20, 2019
@JasonStoltz JasonStoltz requested review from cee-chen and removed request for yakhinvadim September 23, 2019 10:49
@JasonStoltz JasonStoltz added this to the 1.2 milestone Sep 23, 2019
Copy link
Member

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

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

It's been a while and this seems like a pretty meaty PR so I'm going to be going very very slowly in this review! To facilitate that and so I'm not leaving a wall of comments at once I'm going to batch some of my comments into smaller groups or into a couple files at a time. Hope that's not too annoying / gives you a chance to respond to them as I go!

packages/react-search-ui-views/src/SearchBox.js Outdated Show resolved Hide resolved
ADVANCED.md Outdated Show resolved Hide resolved
@JasonStoltz
Copy link
Member Author

@constancecchen Yes, sorry for dragging you into this big PR :( I sincerely appreciate it!

packages/search-ui/src/DebounceManager.js Show resolved Hide resolved
packages/search-ui/src/DebounceManager.js Outdated Show resolved Hide resolved
packages/search-ui/src/DebounceManager.js Outdated Show resolved Hide resolved
packages/search-ui/src/DebounceManager.js Show resolved Hide resolved
packages/search-ui/src/DebounceManager.js Outdated Show resolved Hide resolved
packages/search-ui/src/DebounceManager.js Show resolved Hide resolved
packages/search-ui/src/__tests__/DebounceManager.test.js Outdated Show resolved Hide resolved
packages/search-ui/src/__tests__/DebounceManager.test.js Outdated Show resolved Hide resolved
packages/search-ui/src/__tests__/DebounceManager.test.js Outdated Show resolved Hide resolved
packages/search-ui/src/SearchDriver.js Outdated Show resolved Hide resolved
packages/search-ui/src/SearchDriver.js Show resolved Hide resolved
packages/search-ui/src/SearchDriver.js Outdated Show resolved Hide resolved
packages/search-ui/src/SearchDriver.js Outdated Show resolved Hide resolved
packages/search-ui/src/__tests__/SearchDriver.test.js Outdated Show resolved Hide resolved
packages/search-ui/src/test/helpers.js Show resolved Hide resolved
@JasonStoltz JasonStoltz force-pushed the batch-actions branch 2 times, most recently from 7ed4624 to 5fec228 Compare September 24, 2019 20:29
I updated these tests because they were testing the wrong thing.

There are two side effects that occur as a result of setSearchTerm:
_udpateSearchResults
_makeSearchRequest

_udpateSearchResults is what we are debouncing, so we want to verify
whether that has run or not. Testing whether or not an API request
has been make is not a good way to test this, since _makeSearchRequest
is now always debounced.

In other words, we could call _udpateSearchResults 3 times in a row
and it will execute 3 times, but ultimately only one api call will be
made.
cee-chen
cee-chen previously approved these changes Sep 25, 2019
Copy link
Member

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

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

Fantastic stuff! 🎉

@JasonStoltz
Copy link
Member Author

JasonStoltz commented Sep 25, 2019

@constancecchen This is ready for a re-review.

After giving it some thought, I think we could make a really nice refactor to the SearchDriver.

The idea I had is to clearly distinguish "side effects" inside of the driver, and pull them out into their own modules.

For example, running actions could potentially result in 4 different "side effects".

  • Updating request state (i.e., "_updateSearchResults")
  • Making an API request (i.e., "_makeAPIRequest")
  • Updating A11y notifications
  • Updating the URL state

These could all exist in their own files and be tested independently. It would help a lot cleaning up the driver and making this code more reasonable.

@JasonStoltz
Copy link
Member Author

For another PR though.

@JasonStoltz JasonStoltz merged commit 0ef3535 into elastic:master Sep 26, 2019
@JasonStoltz JasonStoltz deleted the batch-actions branch September 26, 2019 11:36
@JasonStoltz JasonStoltz changed the title Batch actions and the new "clearFilters" option Batch actions and the new "shouldClearFilters" option Sep 26, 2019
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