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

Add fuzzy search hook to @automattic/search #65501

Merged
merged 19 commits into from
Jul 18, 2022
Merged

Conversation

zaguiini
Copy link
Contributor

@zaguiini zaguiini commented Jul 12, 2022

Proposed Changes

Right now, our client-side searches are pretty limited: we have the searchSites HOC that exposes basic search functionality that's not very customizable and the results have to match the term exactly, bringing a subpar user experience.

This PR adds a useFuzzySearch hook that relies on the fuse.js package. That hook can be used anywhere search is needed. You can pass fields that are indexed and the search state is also handled.

Testing Instructions

Check out this branch, run yarn install then yarn build-packages so the @automattic/search package gets updated.

Once done, apply this diff to Calypso:

diff --git a/client/components/search-sites/index.js b/client/components/search-sites/index.js
index e17a05af82..f1fa99da83 100644
--- a/client/components/search-sites/index.js
+++ b/client/components/search-sites/index.js
@@ -1,3 +1,4 @@
+import { useFuzzySearch } from '@automattic/search';
 import { get } from 'lodash';
 import { Component } from 'react';
 import { connect } from 'react-redux';
@@ -16,27 +17,20 @@ const mapState = ( state ) => ( {
 export default function searchSites( WrappedComponent ) {
 	const componentName = WrappedComponent.displayName || WrappedComponent.name || '';
 
-	class Searcher extends Component {
-		state = { term: null };
-
-		setSearchTerm = ( term ) => this.setState( { term } );
-
-		getSearchResults() {
-			return this.state.term
-				? searchCollection( this.props.sites, this.state.term.toLowerCase(), [ 'name', 'URL' ] )
-				: null;
-		}
-
-		render() {
-			return (
-				<WrappedComponent
-					{ ...this.props }
-					searchSites={ this.setSearchTerm }
-					sitesFound={ this.getSearchResults() }
-				/>
-			);
-		}
-	}
+	const Searcher = ( props ) => {
+		const { query, setQuery, results } = useFuzzySearch( {
+			data: props.sites,
+			fields: [ 'name', 'URL' ],
+		} );
+
+		return (
+			<WrappedComponent
+				{ ...props }
+				searchSites={ setQuery }
+				sitesFound={ query ? results : null }
+			/>
+		);
+	};
 
 	const Connected = connect( mapState )( Searcher );
 	Connected.displayName = `SearchSites(${ componentName })`;

Open any site in Calypso, click Switch Site and start typing in the search bar. It no longer requires an exact match to return queried results.

This is an optional change to the searchSites HOC just to exemplify the usage. It does not necessarily need to be used there. I first implemented this hook in #65454 but I moved it here so I don't have a PR breaking the single-subject rule.

Related to #65169, first implemented in #65454.

@zaguiini zaguiini requested a review from a team July 12, 2022 16:28
@zaguiini zaguiini self-assigned this Jul 12, 2022
@matticbot matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jul 12, 2022
@github-actions
Copy link

github-actions bot commented Jul 12, 2022

@matticbot
Copy link
Contributor

matticbot commented Jul 12, 2022

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

Async-loaded Components (~5237 bytes added 📈 [gzipped])

name                          parsed_size              gzip_size
async-load-automattic-search     +15933 B  (+6476.8%)    +5237 B  (+2877.5%)

React components that are loaded lazily, when a certain part of UI is displayed for the first time.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

@danielbachhuber
Copy link
Contributor

This PR adds a useFuzzySearch hook that relies on the fuse.js package. That hook can be used anywhere search is needed. You can pass fields that are indexed and the search state is also handled.

@zaguiini What are some examples where fuzzy search (and a separate library) are advantageous over the pure string match in #65505 ?

@vykes-mac
Copy link
Contributor

vykes-mac commented Jul 12, 2022

I'm not sure what's performance impact but I think fuzzy can be beneficial especially as list of sites grow. I might want to find a site that i do not remember the exact name but entering partial string finds the site for me.

Is the search on cached results or any server call is made?

@vykes-mac
Copy link
Contributor

I actually like this feature, there are particular sites I use at times but after I while it gets lost in recently used order, I often don't remember the name especially with the generated free domain names. Testing this out made it easy to find a site quickly that would otherwise take me sometime.

@zaguiini
Copy link
Contributor Author

zaguiini commented Jul 13, 2022

Thanks, @vykes-mac. That's exactly the case I think fuzzy search should be used for.

Furthermore, there's this great article here exemplifying use cases and why it's a good idea.

So @danielbachhuber, the difference is mostly subjective as there are no performance benefits of one over the other.

Copy link
Contributor

@danielbachhuber danielbachhuber left a comment

Choose a reason for hiding this comment

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

Works great!

image

I don't feel I have any qualified opinions on the JavaScript fanciness, so it'd be good to get a second pair of eyes in that regard.

Also worth noting: I had to add a newline in order for the patch to apply.

package.json Outdated Show resolved Hide resolved
@danielbachhuber danielbachhuber requested a review from a team July 13, 2022 22:28
@zaguiini zaguiini force-pushed the add/fuzzy-search-hook branch 3 times, most recently from 0b3eb29 to dfaf90a Compare July 14, 2022 13:13
@zaguiini
Copy link
Contributor Author

zaguiini commented Jul 14, 2022

@tyxla: added some tests.

One enhancement is now that we're reusing the same Fuse instance whenever data changes, instead of throwing everything away. That improves performance A LOT: a 6x CPU slowdown allows removing debouncing on the <Search /> instance and it still queries very quickly. I'm honestly impressed!

Try it yourself:

diff --git a/client/components/site-selector/index.jsx b/client/components/site-selector/index.jsx
index e010714eb9..a085c03c9e 100644
--- a/client/components/site-selector/index.jsx
+++ b/client/components/site-selector/index.jsx
@@ -416,7 +416,6 @@ export class SiteSelector extends Component {
 			>
 				<SearchComponent
 					onSearch={ this.onSearch }
-					delaySearch={ true }
 					placeholder={ this.props.searchPlaceholder }
 					// eslint-disable-next-line jsx-a11y/no-autofocus
 					autoFocus={ this.props.autoFocus }

Then open the sidebar > site search in Calypso.

Please, re-review the PR!

Copy link
Contributor

@danielbachhuber danielbachhuber left a comment

Choose a reason for hiding this comment

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

Works well in my testing! Left some non-blocking comments.

packages/search/src/index.ts Show resolved Hide resolved
packages/search/src/test/use-fuzzy-search.tsx Show resolved Hide resolved
packages/search/src/use-fuzzy-search.ts Show resolved Hide resolved
Copy link
Member

@p-jackson p-jackson left a comment

Choose a reason for hiding this comment

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

Looks like Fuse supports some powerful searching. Maybe in the future we'll want to expose the scores from the hook too, but we can do that when the need arises.

fuse.js seems solid enough from a maintenance point of view. Apache licensed.

packages/search/src/index.ts Show resolved Hide resolved
packages/search/src/use-fuzzy-search.ts Outdated Show resolved Hide resolved
packages/search/tsconfig.json Outdated Show resolved Hide resolved
Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

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

Looking good 👍

Left a few comments, nothing blocking, and this can be shipped with or without addressing them IMHO.

Nice work 🚀

packages/search/src/test/use-fuzzy-search.tsx Outdated Show resolved Hide resolved
packages/search/src/test/use-fuzzy-search.tsx Outdated Show resolved Hide resolved
packages/search/src/test/use-fuzzy-search.tsx Outdated Show resolved Hide resolved
packages/search/src/use-fuzzy-search.ts Show resolved Hide resolved
@zaguiini zaguiini force-pushed the add/fuzzy-search-hook branch 2 times, most recently from 7c86ff6 to cf3a033 Compare July 15, 2022 18:06
@zaguiini zaguiini force-pushed the add/fuzzy-search-hook branch from cf3a033 to b406375 Compare July 18, 2022 13:01
@zaguiini
Copy link
Contributor Author

@tyxla @danielbachhuber @vykes-mac @p-jackson would appreciate a review of the README changes before merging this PR!

Copy link
Contributor

@danielbachhuber danielbachhuber left a comment

Choose a reason for hiding this comment

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

README update looks great!

packages/search/README.md Outdated Show resolved Hide resolved
packages/search/README.md Outdated Show resolved Hide resolved
Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

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

Still looking great! Thanks for the thorough README and for the tests.

Just left a couple of little suggestions, but nothing blocking.

🚀

packages/search/package.json Show resolved Hide resolved
packages/search/src/test/use-fuzzy-search.tsx Outdated Show resolved Hide resolved
@zaguiini zaguiini merged commit a429aff into trunk Jul 18, 2022
@zaguiini zaguiini deleted the add/fuzzy-search-hook branch July 18, 2022 13:47
@github-actions github-actions bot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jul 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants