Search is not working

[erakli]

Hello,

Thank you for this fork, looks promising.

While I tried to integrate RST into my application I found some difficulties. One of them is that search is not working as expected. I tried to fully reproduce this story, but with current code it is not working (nodes aren't highlightd, focus is not working).

https://user-images.githubusercontent.com/11831470/147665981-4624c7e1-cb40-40b3-ba76-26ff314aa53c.mov

The only workaround to see searched nodes is to use onlyExpandSearchedNodes.

As I can see, the most problematic place is the getDerivedStateFromProps() method. It is really hard to follow data flow for state members searchMatches and searchFocusTreeIndex. For my use case I made quick fix (inspired by this blog post) that do memoized computation of both values in render() and it works

https://user-images.githubusercontent.com/11831470/147665989-bfe1739b-7de5-4551-8ed0-973173f5c7f8.mov

This solution isn't perfect, because there are couple of re-renders.

Example patch:

Index: src/react-sortable-tree.tsx
<+>UTF-8
===================================================================
diff --git a/src/react-sortable-tree.tsx b/src/react-sortable-tree.tsx
--- a/src/react-sortable-tree.tsx   
+++ b/src/react-sortable-tree.tsx   
@@ -7,6 +7,7 @@
   createVerticalStrength,
 } from '@nosferatu500/react-dnd-scrollzone'
 import isEqual from 'lodash.isequal'
+import memoize from 'memoize-one'
 import { DndContext, DndProvider } from 'react-dnd'
 import { HTML5Backend } from 'react-dnd-html5-backend'
 import { Virtuoso } from 'react-virtuoso'
@@ -303,22 +305,8 @@
       newState.draggedMinimumTreeIndex = undefined
       newState.draggedDepth = undefined
       newState.dragging = false
-    } else if (!isEqual(instanceProps.searchQuery, nextProps.searchQuery)) {
-      Object.assign(
-        newState,
-        ReactSortableTree.search(nextProps, prevState, true, true, false)
-      )
-    } else if (
-      instanceProps.searchFocusOffset !== nextProps.searchFocusOffset
-    ) {
-      Object.assign(
-        newState,
-        ReactSortableTree.search(nextProps, prevState, true, true, true)
-      )
     }

-    instanceProps.searchQuery = nextProps.searchQuery
-    instanceProps.searchFocusOffset = nextProps.searchFocusOffset
     newState.instanceProps = { ...instanceProps, ...newState.instanceProps }

     return newState
@@ -621,24 +609,61 @@
     )
   }

+  memoizedSearch = memoize((props, state, seekIndex, expand, singleSearch) =>
+    ReactSortableTree.search(props, state, seekIndex, expand, singleSearch)
+  )
+
   render() {
+    const {
+      onChange,
+      getNodeKey,
+      searchFinishCallback,
+      searchQuery,
+      searchMethod,
+      searchFocusOffset,
+      onlyExpandSearchedNodes,
+    } = this.props
+    const { instanceProps } = this.state
+
+    let flags = [false, false, false]
+    if (!isEqual(instanceProps.searchQuery, searchQuery)) {
+      flags = [true, true, false]
+      this.setState({ instanceProps: { ...instanceProps, searchQuery } })
+    } else if (instanceProps.searchFocusOffset !== searchFocusOffset) {
+      flags = [true, true, true]
+      this.setState({ instanceProps: { ...instanceProps, searchFocusOffset } })
+    }
+
+    const { searchMatches, searchFocusTreeIndex } = this.memoizedSearch(
+      {
+        onChange,
+        getNodeKey,
+        searchFinishCallback,
+        searchQuery,
+        searchMethod,
+        searchFocusOffset,
+        onlyExpandSearchedNodes,
+      },
+      { instanceProps },
+      ...flags
+    )
+
     const {
       dragDropManager,

Also, I'm not very convenient with class components, so I'm not sure that setState() in render is a good idea.

Summary and solutions

• we could make a quick fix for now
• probably there could be more correct solution (fix something in virtual scroll, etc)
• remove getDerivedStateFromProps() at all, make ReactSortableTree controlled in the way, that parent component controlls the search
• use hooks?

[ndanvers]

Are you noticing search issues while strict mode is on?

Original RST notes this as a deficiency, try removing strict mode (or create a production build) and see if search is working as expected

On Wed, Dec 29, 2021 at 10:23 PM Egor Panfilov @.***> wrote:

Hello,

Thank you for this fork, looks promising.

While I tried to integrate RST into my application I found some difficulties. One of them is that search is not working as expected. I tried to fully reproduce this story https://frontend-collective.github.io/react-sortable-tree/?path=/story/basics--search, but with current code it is not working (nodes aren't highlightd, focus is not working).

https://user-images.githubusercontent.com/11831470/147665981-4624c7e1-cb40-40b3-ba76-26ff314aa53c.mov

The only workaround to see searched nodes is to use onlyExpandSearchedNodes.

As I can see, the most problematic place is the getDerivedStateFromProps() method. It is really hard to follow data flow for state members searchMatches and searchFocusTreeIndex. For my use case I made quick fix (inspired by this blog post https://reactjs.org/blog/2018/06/07/you-probably-dont-need-derived-state.html) that do memoized computation of both values in render() and it works

https://user-images.githubusercontent.com/11831470/147665989-bfe1739b-7de5-4551-8ed0-973173f5c7f8.mov

This solution isn't perfect, because there are couple of re-renders.

Example patch:

Index: src/react-sortable-tree.tsx <+>UTF-8===================================================================diff --git a/src/react-sortable-tree.tsx b/src/react-sortable-tree.tsx--- a/src/react-sortable-tree.tsx +++ b/src/react-sortable-tree.tsx @@ -7,6 +7,7 @@ createVerticalStrength, } from @.***/react-dnd-scrollzone' import isEqual from 'lodash.isequal'+import memoize from 'memoize-one' import { DndContext, DndProvider } from 'react-dnd' import { HTML5Backend } from 'react-dnd-html5-backend' import { Virtuoso } from 'react-virtuoso'@@ -303,22 +305,8 @@ newState.draggedMinimumTreeIndex = undefined newState.draggedDepth = undefined newState.dragging = false- } else if (!isEqual(instanceProps.searchQuery, nextProps.searchQuery)) {- Object.assign(- newState,- ReactSortableTree.search(nextProps, prevState, true, true, false)- )- } else if (- instanceProps.searchFocusOffset !== nextProps.searchFocusOffset- ) {- Object.assign(- newState,- ReactSortableTree.search(nextProps, prevState, true, true, true)- ) }

• instanceProps.searchQuery = nextProps.searchQuery- instanceProps.searchFocusOffset = nextProps.searchFocusOffset newState.instanceProps = { ...instanceProps, ...newState.instanceProps }

  return newState@@ -621,24 +609,61 @@ ) }

• memoizedSearch = memoize((props, state, seekIndex, expand, singleSearch) =>+ ReactSortableTree.search(props, state, seekIndex, expand, singleSearch)+ )+ render() {+ const {+ onChange,+ getNodeKey,+ searchFinishCallback,+ searchQuery,+ searchMethod,+ searchFocusOffset,+ onlyExpandSearchedNodes,+ } = this.props+ const { instanceProps } = this.state++ let flags = [false, false, false]+ if (!isEqual(instanceProps.searchQuery, searchQuery)) {+ flags = [true, true, false]+ this.setState({ instanceProps: { ...instanceProps, searchQuery } })+ } else if (instanceProps.searchFocusOffset !== searchFocusOffset) {+ flags = [true, true, true]+ this.setState({ instanceProps: { ...instanceProps, searchFocusOffset } })+ }++ const { searchMatches, searchFocusTreeIndex } = this.memoizedSearch(+ {+ onChange,+ getNodeKey,+ searchFinishCallback,+ searchQuery,+ searchMethod,+ searchFocusOffset,+ onlyExpandSearchedNodes,+ },+ { instanceProps },+ ...flags+ )+ const { dragDropManager,

Also, I'm not very convenient with class components, so I'm not sure that setState() in render is a good idea. Summary and solutions

• we could make a quick fix for now
• probably there could be more correct solution (fix something in virtual scroll, etc)
• remove getDerivedStateFromProps() at all, make ReactSortableTree controlled in the way, that parent component controlls the search
• use hooks?

— Reply to this email directly, view it on GitHub https://github.com/nosferatu500/react-sortable-tree/issues/11, or unsubscribe https://github.com/notifications/unsubscribe-auth/AP4PE3TCGYQIKNRMOUXCXQ3UTMDUVANCNFSM5K56MGDA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you are subscribed to this thread.Message ID: @.***>

-- Nicholas Danvers Software Engineer

https://www.imagineeverything.com/

[nosferatu500]

Everything works fine on dev and prod builds.

[erakli]

@ndanvers, thanks for reply. Sorry for late response.

Original RST notes this as a deficiency, try removing strict mode (or create a production build) and see if search is working as expected

You definitely right. I've checked the search in production build and it is working as expected. In development build search it is still not working in v4.0.4. By the way, I'm using this fork of RST in JS code not TS.

@nosferatu500, I think some sort of disclaimer about search in dev builds would be fine. What do you think?

[nosferatu500]

@erakli What build system are you having this problem on? On my side, (with vite) search works as expected in both dev and prod builds.

[erakli]

@nosferatu500 I'm using a project bootstrapped with CRA so everything that shipped with it I'm using. There is webpack I suppose.