Skip to content

Search improvements #572

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Jan 29, 2018
Merged

Search improvements #572

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
28c4d81
Search improvements
Jan 26, 2018
eb1be2c
Merge branch 'master' into 550-search-improvements
Jan 28, 2018
cd29487
Comments + some cleaning up
Jan 28, 2018
a99557d
Typo
Jan 28, 2018
e0e4c6e
Missing depth argument
Jan 28, 2018
e4a8966
Clear seen Map
Jan 28, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion shells/dev/target/NativeTypes.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ export default {

theStore () {
return this.$store
},
}
},

mounted () {
Expand Down
16 changes: 5 additions & 11 deletions src/devtools/components/DataField.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,8 @@ import {
isPlainObject,
sortByKey,
openInEditor,
escape
escape,
specialTokenToString
} from 'src/util'

import DataFieldEdit from '../mixins/data-field-edit'
Expand Down Expand Up @@ -261,18 +262,11 @@ export default {

formattedValue () {
const value = this.field.value
let result
if (this.fieldOptions.abstract) {
return ''
} else if (value === null) {
return 'null'
} else if (value === UNDEFINED) {
return 'undefined'
} else if (value === NAN) {
return 'NaN'
} else if (value === INFINITY) {
return 'Infinity'
} else if (value === NEGATIVE_INFINITY) {
return '-Infinity'
} else if ((result = specialTokenToString(value))) {
return result
} else if (this.valueType === 'custom') {
return value._custom.display
} else if (this.valueType === 'array') {
Expand Down
91 changes: 66 additions & 25 deletions src/util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,21 @@ export const SPECIAL_TOKENS = {
'NaN': NAN
}

export function specialTokenToString (value) {
if (value === null) {
return 'null'
} else if (value === UNDEFINED) {
return 'undefined'
} else if (value === NAN) {
return 'NaN'
} else if (value === INFINITY) {
return 'Infinity'
} else if (value === NEGATIVE_INFINITY) {
return '-Infinity'
}
return false
}

/**
* Needed to prevent stack overflow
* while replacing complex objects
Expand Down Expand Up @@ -312,44 +327,70 @@ function isPrimitive (data) {
}

export function searchDeepInObject (obj, searchTerm) {
var match = false
return internalSearchObject(obj, searchTerm.toLowerCase(), new Map(), 0)
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be safer to clear the map when leaving the function to avoid holding references to objects and creating a leak

}

function internalSearchObject (obj, searchTerm, seen, depth) {
let match = false
const keys = Object.keys(obj)
let key, value
for (let i = 0; i < keys.length; i++) {
const key = keys[i]
const value = obj[key]
if (compare(key, searchTerm) || compare(value, searchTerm)) {
match = true
key = keys[i]
value = obj[key]
match = interalSearchCheck(searchTerm, key, value, seen, depth + 1)
if (match) {
break
}
if (isPlainObject(value)) {
match = searchDeepInObject(value, searchTerm)
if (match) {
break
}
}
return match
}

function internalSearchArray (array, searchTerm, seen, depth) {
let match = false
let value
for (let i = 0; i < array.length; i++) {
value = array[i]
match = interalSearchCheck(searchTerm, null, value, seen, depth + 1)
if (match) {
break
}
}
return match
}

function compare (mixedValue, stringValue) {
if (Array.isArray(mixedValue) && searchInArray(mixedValue, stringValue.toLowerCase())) {
return true
function interalSearchCheck (searchTerm, key, value, seen, depth) {
if (depth > 10) {
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's what the seen Map is for.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

For performance reasons, I though it would be nice to have some kind of limit.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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 = b

we would only dive till a.b.b (or a.b.b.a depending on the implem)

Copy link
Member Author

Choose a reason for hiding this comment

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

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).

Copy link
Member

Choose a reason for hiding this comment

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

Ah, okay so it was to limit deep nested objects no matter repetition.
Forget about WeakMap, Map is ok 🙂

return false
}
if (('' + mixedValue).toLowerCase().indexOf(stringValue.toLowerCase()) !== -1) {
return true
let match = false
let result
if (key === '_custom') {
key = value.display
value = value.value
}
return false
(result = specialTokenToString(value)) && (value = result)
if (key && compare(key, searchTerm)) {
match = true
seen.set(value, true)
} else if (seen.has(value)) {
match = seen.get(value)
} else if (Array.isArray(value)) {
seen.set(value, null)
match = internalSearchArray(value, searchTerm, seen)
seen.set(value, match)
} else if (isPlainObject(value)) {
seen.set(value, null)
match = internalSearchObject(value, searchTerm, seen)
seen.set(value, match)
} else if (compare(value, searchTerm)) {
match = true
seen.set(value, true)
}
return match
}

function searchInArray (arr, searchTerm) {
let found = false
for (let i = 0; i < arr.length; i++) {
if (('' + arr[i]).toLowerCase().indexOf(searchTerm) !== -1) {
found = true
break
}
}
return found
function compare (value, stringValue) {
return ('' + value).toLowerCase().indexOf(stringValue) !== -1
}

export function sortByKey (state) {
Expand Down