Trim URLs in breadcrumbs; add maxUrlLength config opt
#906
Conversation
| crumb, | ||
| data; | ||
|
|
||
| for (var i = 0; i < breadcrumbs.values.length; i++) { |
There was a problem hiding this comment.
possible improvement would be truncating differently based on how many total breadcrumbs there are (maxurl length = (12500 / breadcrumbs.values.length)) but definitely not a big deal
There was a problem hiding this comment.
this would make it harder to search for exact matches in breadcrumb urls though so shrug
There was a problem hiding this comment.
Nah we're not gonna get that fancy. I'd sooner drop the maxBreadcrumbs default to 50.
| continue; | ||
|
|
||
| data = crumb.data; | ||
| for (var prop in urlprops) { |
There was a problem hiding this comment.
@benvinegar late to the party here but this is slightly unsafe - if Object.prototype has a property added to it and data has an own property of that same name, we might truncate something we don't mean to. Simplest fix is to add && prop.hasOwnProperty(prop) but I'm not sure why we don't just make urlprops an array and iterate through. Call it one way or the other and I'll make a PR.
There was a problem hiding this comment.
Seems low risk, but yeah, I agree.
I'm not sure why we don't just make urlprops an array and iterate through
I was feeling fancy. Let's just go with array.
Fixes #874 (sort of)
cc @LewisJEllis @MaxBittker