Skip to content

Conversation

@gandhis1
Copy link
Contributor

@gandhis1 gandhis1 commented Jul 13, 2022

What does it do?

When you have tens of thousands of items in a search tree, and you type search input, the cost can be extremely high when you have zero or one characters, especially when you are using a custom search predicate. There is no need to incur the cost of filtering the tree if the text input is zero length. Instead, just reset the search state.

I'll note here that the example in the demo that is supposed to test a large-scale scenario doesn't appear to be all that taxing because the number of nested nodes is very low. You need a lot of nested nodes to encounter performance issues. In my testing, I have almost 100k nodes at up to 10 levels of nesting. I also have keepTreeOnSearch on which probably exacerbates the issue.

This is the first change I would like to do. In a future PR, my intention would be to set the threshold (search term length) at which the search activates. When your search term is "a" you can trigger rendering of thousands of nodes potentially. So this parameter I am envisioning would basically prevent search mode until X characters...with some kind of visual indicator.

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Checklist:

  • I have performed a self-review of my own code
  • have commented my code, particularly in hard-to-understand areas
  • Updated documentation (if applicable)
  • Added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • My changes generate no new warnings

@gandhis1 gandhis1 changed the title Do not incur cost of filtering tree if search mode is off feat: Do not incur cost of filtering tree if search mode is off Jul 13, 2022
@gandhis1 gandhis1 force-pushed the search_short_circuit branch from e57d4ef to de56dfe Compare July 13, 2022 15:53
@gandhis1
Copy link
Contributor Author

Tagging @mrchief

@gandhis1
Copy link
Contributor Author

As a temporary step, I ended up forking this package and publishing my own: https://www.npmjs.com/package/@gandhis1/react-dropdown-tree-select

I then tested it in my main application and this fix works exactly as intended. 100k nodes across 10+ levels does not lock up when you have an empty string search term.

@mrchief
Copy link
Collaborator

mrchief commented Jul 27, 2022

@gandhis1 Thanks for sending this! This sounds like a great idea and I'll look into this soon. Been swamped at work lately

@gandhis1
Copy link
Contributor Author

gandhis1 commented Aug 6, 2022

Hi any update on this? It's a pretty tiny PR

@mrchief
Copy link
Collaborator

mrchief commented Aug 18, 2022

Sorry @gandhis1 Will do by end of the week

@github-actions
Copy link

github-actions bot commented Oct 2, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 10 days if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale label Oct 2, 2022
@gandhis1
Copy link
Contributor Author

gandhis1 commented Oct 2, 2022

Following up, can this be reviewed?

@stale stale bot removed the stale label Oct 2, 2022
@mrchief
Copy link
Collaborator

mrchief commented Oct 4, 2022

@gandhis1 I'm swamped at the moment but I plan on getting to this, along with the other stuff that's pending. Thanks for waiting.

@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 10 days if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale label Nov 18, 2022
@gandhis1
Copy link
Contributor Author

Posting to keep this from being stale, PR is awaiting review

@stale stale bot removed the stale label Nov 18, 2022
@github-actions
Copy link

github-actions bot commented Jan 3, 2023

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 10 days if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale label Jan 3, 2023
@gandhis1
Copy link
Contributor Author

gandhis1 commented Jan 4, 2023

Not stale

@stale stale bot removed the stale label Jan 4, 2023
Copy link
Collaborator

@mrchief mrchief left a comment

Choose a reason for hiding this comment

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

Sorry for the delay and thank for sending this

@mrchief mrchief merged commit 68aa40a into dowjones:main Jan 15, 2023
mrchief added a commit that referenced this pull request Jan 15, 2023
Co-authored-by: Siddhartha Gandhi <siddhartha.gandhi@centivacapital.com>
Co-authored-by: Hrusikesh Panda <mrchief@users.noreply.github.com>
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