Refactored all add() calls from SortedSet to only pass single values #203
Conversation
before this fix, the index passed all tokens via an apply call, pushing all parameters on the stack. This behaviour causes a RangeError when more tokens are added than the JS implementation can handle (Firefox 44 -> 262143, Node.js -> 65535, evaluated using this test: http://stackoverflow.com/questions/22747068/is-there-a-max-number-of-arguments-javascript-functions-can-accept) this fix changes the code so that now only a single value gets passed per call. This way no change to the current API is neccesary. the code was refactored in a way that it now passes only a single argument at a time, avoiding the usage of apply The only parts of the code that still use apply are the EventEmitters and the Plugin subsystem which should be non critical for this type of error.
| @@ -224,7 +224,9 @@ lunr.SortedSet.prototype.union = function (otherSet) { | |||
|
|
|||
| unionSet = longSet.clone() | |||
|
|
|||
| unionSet.add.apply(unionSet, shortSet.toArray()) | |||
| for(var i = 0, shortSetElements = shortSet.toArray(); i < shortSetElements.length; i++){ | |||
| unionSet.add(shortSetElements[i]); | |||
There was a problem hiding this comment.
tiny nitpick - none of the other code includes explicitly semicolons. We should probably try and match that style.
|
One tiny comment, otherwise the change looks good. |
the rest of the project is coded in a semicolon free style. this commit makes a change to match that style in one location
|
Thanks I just fixed the line. Maybe I can add a .jshintrc with the lastsemic option set at some point? That would at least supress all warnings for a automatic linter like I have in Atom. |
I agree, I prefer JavaScript with semicolons. Anyway, this change looks good to me now! 👍 |
|
This is merged in the latest release now, thanks a lot the fix. I actually think adding some kind of linter to the build would be a fantastic idea, it certainly helps any would be contributors to get their patch applied quicker (sadly I'm probably still the bottleneck though!) and would help ensure a decent level of consistency in the code base. Also semicolon free style is clearly better... |
🔥
Before this fix, the index passed all tokens via an apply call, pushing all parameters on the stack.
This behaviour causes a RangeError when more tokens are added than the JS implementation can handle (Firefox 44 -> 262143, Node.js -> 65535, evaluated using this test: http://stackoverflow.com/questions/22747068/is-there-a-max-number-of-arguments-javascript-functions-can-accept)
this fix changes the code so that now only a single value gets passed per call. This way no change to the current API is neccesary.
The code was refactored in a way that it now passes only a single argument at a time, avoiding the usage of apply.
The only parts of the code that still use apply are the EventEmitters and the Plugin subsystem which should be non critical for this type of error.