-
Notifications
You must be signed in to change notification settings - Fork 10
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
Replace search engine and improve UX #337
Conversation
✅ Deploy Preview for cucumber-react-components ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
# Conflicts: # package-lock.json # package.json # src/components/app/FilteredResults.spec.tsx # src/components/app/FilteredResults.tsx # src/filter/filterByStatus.spec.ts # src/search/FeatureSearch.spec.ts # src/search/RuleSearch.spec.ts # src/search/ScenarioSearch.spec.ts # src/search/Search.spec.ts # src/search/Search.ts # src/search/StepSearch.spec.ts # src/search/TagSearch.spec.ts # src/search/TextSearch.spec.ts # src/search/TextSearch.ts
2d57890
to
f530426
Compare
# Conflicts: # .github/workflows/test.yml # package-lock.json # src/components/app/FilteredResults.spec.tsx # src/filter/filterByStatus.spec.ts # src/search/FeatureSearch.spec.ts # src/search/RuleSearch.spec.ts # src/search/ScenarioSearch.spec.ts # src/search/Search.spec.ts # src/search/StepSearch.spec.ts # src/search/TagSearch.spec.ts
* Can be used for Backgrounds, Scenarios and Rules, searching against the | ||
* name and description | ||
*/ | ||
class ScenarioLikeSearch<T extends ScenarioLike> implements TypedIndex<T> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uses duck typing to avoid the need for specific classes for Scenario, Rule or Background
@@ -16,12 +19,12 @@ export default function filterByStatus( | |||
const pickleIds = gherkinQuery.getPickleIds(gherkinDocument.uri, scenario.id) | |||
|
|||
return pickleIds | |||
.map((pickleId) => | |||
.filter((pickleId) => envelopesQuery.hasTestCase(pickleId)) | |||
.some((pickleId) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More efficient
@@ -27,28 +37,15 @@ export default function countScenariosByStatuses( | |||
} { | |||
const scenarioCountByStatus = makeEmptyScenarioCountsByStatus() | |||
|
|||
for (const gherkinDocument of gherkinQuery.getGherkinDocuments()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is totals for the whole test run, it's simpler and faster to interrogate cucumber data directly than go through all the Gherkin documents.
c0d0554
to
b98596d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was only needed to work around elasticlunr
issues.
/> | ||
</div> | ||
|
||
{filtered.length > 0 && <GherkinDocumentList gherkinDocuments={filtered} preExpand={true} />} | ||
{filtered.length < 1 && <NoMatchResult query={query} />} | ||
{filtered !== undefined && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
filtered
can be undefined if the (async) search initialisation hasn't happened yet - in which case we just won't render anything yet in this area.
@@ -52,15 +36,22 @@ export const FilteredResults: React.FunctionComponent<IProps> = ({ className }) | |||
/> | |||
<SearchBar | |||
query={query} | |||
onSearch={(query) => update({ query })} | |||
onSearch={(newValue) => update({ query: newValue })} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was shadowing a variable higher in the scope.
statusesWithScenarios={statusesWithScenarios} | ||
hideStatuses={hideStatuses} | ||
onFilter={(hideStatuses) => update({ hideStatuses })} | ||
onFilter={(newValue) => update({ hideStatuses: newValue })} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above.
const formData = new window.FormData(event.currentTarget) | ||
const query = formData.get('query') | ||
onSearch((query || '').toString()) | ||
debouncedSearchChange.flush() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
flush()
== "fast forward and apply any pending changes now"
Recent changes to `@cucumber/react-components` [1] makes rendering async. This is a good opportunity to switch to a library providing query polling. [1] cucumber/react-components#337
Recent changes to `@cucumber/react-components` [1] makes rendering async. This is a good opportunity to switch to a library providing query polling. [1] cucumber/react-components#337
🤔 What's changed?
elasticlunr
is removed in favour of Orama. This gets rid of bundling issues and allows us to do a production build of Ladle and deploy the preview with Netlify. It also reduces the dependency footprint (see lockfile diff).The diff is bigger than you might expect for simply swapping out a library that does the same thing, because all Orama's interfaces (for creating indexes, and inserting, and searching) are async. This required a refactor of how and when we build the search to fit that async pattern. As such, lot of tests needed converting to be async and having assertions wrapped in
await waitFor()
etc.Also, search plumbing is now extracted from the
<FilteredResults/>
component and done in a hookuseFilteredDocuments()
.Finally, applied the search query on change (with half-second debounce) so you don't have to hit enter each time - this often frustrates me as a user.
⚡️ What's your motivation
elasticlunr
is unmaintained, and also published in a format that is troublesome for some module bundlers e.g. Vite that is behind Ladle. We need to replace it with something more modern, and while we're at it move logic out of components and into hooks/functions where possible.🏷️ What kind of change is this?
📋 Checklist:
This text was originally generated from a template, then edited by hand. You can modify the template here.