Search improvements #572
Search improvements #572
Conversation
src/util.js
Outdated
| if (Array.isArray(mixedValue) && searchInArray(mixedValue, stringValue.toLowerCase())) { | ||
| return true | ||
| function interalSearchCheck (searchTerm, key, value, seen, depth) { | ||
| if (depth > 10) { |
There was a problem hiding this comment.
It's okay for a hotfix, but wouldn't it be better to use something like WeakMap to make sure we're not checking an object we have checked before?
There was a problem hiding this comment.
It's what the seen Map is for.
There was a problem hiding this comment.
oh lol sorry, I thought we couldn't use objects as keys in js maps, not sure why.
Why do we wait for depth to be 10, wouldn't be safe to stop if we check the same object twice while inspecting an object?
There was a problem hiding this comment.
For performance reasons, I though it would be nice to have some kind of limit.
There was a problem hiding this comment.
I thought we couldn't use objects as keys in js maps
It's the other way around, we can use objects in Map but not in WeakMap.
There was a problem hiding this comment.
I've used WeakMap with objects before 😆
About the depth, I don't get what you mean with performance, if we drop the search because we found an object we already found while looking recursively into anothe object, wouldn't that be the sortest way to stop the search?
// given
{ a: { b: obj }, c: obj}once we try to lookup obj inside of b, we would never look it up inside of c
// given
var a = {}
var b = { b: { a }}
a.b = bwe would only dive till a.b.b (or a.b.b.a depending on the implem)
There was a problem hiding this comment.
Okay, I don't why I had errors with WeakMap.
If you don't have any match, we could spend a lot of time on very large objects, I thing it's a good idea to have some kind of hard limit on recursive depth (also, the algorithm can search deeply into arrays now).
There was a problem hiding this comment.
Ah, okay so it was to limit deep nested objects no matter repetition.
Forget about WeakMap, Map is ok 🙂
src/util.js
Outdated
| export function searchDeepInObject (obj, searchTerm) { | ||
| var match = false | ||
| return internalSearchObject(obj, searchTerm.toLowerCase(), new Map(), 0) |
There was a problem hiding this comment.
I think it would be safer to clear the map when leaving the function to avoid holding references to objects and creating a leak
seenMap