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

feat(component-testing): Fuzzy search #15546

Closed
wants to merge 7 commits into from
Closed

Conversation

agg23
Copy link
Contributor

@agg23 agg23 commented Mar 17, 2021

Naive fuzzy search for CT. Contains only the most basic configuration, and does not have match highlighting. The useFuzzySearch hook does expose the necessary information, but I was uncomfortable with the current component implementation, and decided to leave that for another PR.

I will be adding Jest tests for useFuzzySearch. They may come in this PR (which requires waiting for a separate PR setting up Jest), or in a future PR as I shore up the design system.

We may want to debounce the filtering, but I don't see the need at the moment.

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Mar 17, 2021

Thanks for taking the time to open a PR!

@cypress
Copy link

cypress bot commented Mar 17, 2021



Test summary

9392 0 119 3Flakiness 1


Run details

Project cypress
Status Passed
Commit 0ff484b
Started Mar 17, 2021 8:43 PM
Ended Mar 17, 2021 8:54 PM
Duration 11:10 💡
OS Linux Debian - 10.5
Browser Multiple

View run in Cypress Dashboard ➡️


Flakiness

cypress/integration/retries.ui.spec.js Flakiness
1 runner/cypress retries.ui.spec > opens attempt on each attempt failure for the screenshot, and closes after test passes

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

Copy link
Contributor

@lmiller1990 lmiller1990 left a comment

Choose a reason for hiding this comment

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

Seems legit, I did not pull it to test locally yet. It might be good to add a minimal test for the searching (probably inside of RunnerCt, it should be quite easy).

Also looks like something got busted in Design System that is breaking the tests there.

npm/design-system/src/library/hooks/useFuzzySearch.ts Outdated Show resolved Hide resolved
packages/runner-ct/src/app/RunnerCt.tsx Show resolved Hide resolved
Copy link
Contributor

@dmtrKovalenko dmtrKovalenko left a comment

Choose a reason for hiding this comment

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

  1. Please move the logic hook out of design system.
  2. We also need to highlight matched letters on the files/folders. For example when you have one top-level folder called tests and you are typing tes you will see that nothing changed. Just a UX imporvement

@@ -0,0 +1,74 @@
import Fuse from 'fuse.js'
import { useEffect, useLayoutEffect, useMemo, useState } from 'react'
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
import { useEffect, useLayoutEffect, useMemo, useState } from 'react'
import * as React from 'react'

What do you think about this? Actually, this is the only right way to import React as ESM module. Moreover it is much easier to read code when default react hooks are started with React.

facebook/react#18102
https://twitter.com/sebmarkbage/status/1231673639080038400

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, this is the only right way to import React as ESM module

Is this actually relevant to us? Why would it matter?

much easier to read code when default react hooks are started with React.

I disagree with this and see no significant difference other than styling. I will not be changing it for your (CT) codebase, but for design-system I will continue importing React as above.

Copy link
Contributor

Choose a reason for hiding this comment

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

I did post 2 links. You are doing actually an invalid thing, you are trying to import UMD using esm default export. Once react will finally have esm export this will not be possible. So it's not the styling.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know very little about the crazy mess that is different types of JS modules, but the most recent React team interaction on modules/importing was the following: https://reactjs.org/blog/2020/09/22/introducing-the-new-jsx-transform.html#removing-unused-react-imports

In particular, I will point out this:

Change all default React imports (i.e. import React from "react") to destructured named imports (ex. import { useState } from "react") which is the preferred style going into the future. This codemod will not affect the existing namespace imports (i.e. import * as React from "react") which is also a valid style. The default imports will keep working in React 17, but in the longer term we encourage moving away from them.

As a question of my own, don't import * imports hurt tree shaking?

Copy link
Contributor

Choose a reason for hiding this comment

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

As a question of my own, don't import * imports hurt tree shaking?

No, just because for now it's still an all-piece of react in one object that just exports different parts. But yeah export default is the biggest mess.

@lmiller1990
Copy link
Contributor

lmiller1990 commented Mar 19, 2021

I tried this locally. Fuse.js gives really terrible results. This is intended apparently - see this comment by the author for example.

If you have file-1 and file-2, querying for file-1 will return both. This is because "file-2 is close to file-1" according to the author. While this may be true, this is absolutely not the expected behavior in a text editor search. I tweaked the config a LOT but no luck - even in our own tiny design system, the Fuse.js results are really not good. The results are totally different compared to the results vscode returns when using the CMD + P feature, for example.

One lib I have used in the past is fuzzysort which gives you exactly what you'd expect with 0 config. Any particular reason to use Fuse.js?

@dmtrKovalenko
Copy link
Contributor

BTW I've been using fuzzy-search here https://material-ui-pickers.dev/api/DatePicker (for props search and it can be 50+ props here) it works really nice for years and can search through object, so we can search even through the whole relative path.

Sources https://github.com/mui-org/material-ui-pickers/blob/next/docs/_shared/PropTypesTable.tsx

@lmiller1990
Copy link
Contributor

lmiller1990 commented Mar 19, 2021

Neat... I made a simple implementation with https://github.com/farzher/fuzzysort and it seems great.

Let's discuss more. I think my overall implementation is pretty bad so we can probably trash it in the future. I used the wrong data structure (nested tree instead of flat) and it made everything really hard to work with.

@agg23
Copy link
Contributor Author

agg23 commented Mar 19, 2021

I am perfectly fine with fuzzysort, I've used it for years. I moved over to Fuse because fuzzysort also has some limitations (though I can no longer remember what they were) and I found the matching to be more complete with fuse.js.

On the topic of fuse.js itself, at least for the exact match functionality, one can trivially do a quick find to check for exact matches.

I would recommend against fuzzy-search, as it appears it does not support weighted scoring.

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