Improved solution to issue #179 (invalid options throwing errors in joinRegExp) by johnbacon · Pull Request #180 · getsentry/sentry-javascript

johnbacon · 2014-01-11T23:01:15Z

This slightly more robust solution ignores options that are not strings
or regular expressions in the passed patterns array.

The prior solution still accepted non-string, non-regexp options (e.g.
[], {}, true, etc.).

Credit goes to @soundslocke for the fix.

@soundslocke

…rors in joinRegExp) This slightly more robust solution ignores options that are not strings or regular expressions in the passed patterns array. The prior solution still accepted non-string, non-regexp options (e.g. [], {}, true, etc.). Credit goes to @soundslocke for the fix.

mattrobenolt · 2014-01-11T23:21:46Z

Is it necessary to check again for isUndefined? Wouldn't just patterns[i] && patterns[i].source be enough?

mattrobenolt · 2014-01-11T23:24:11Z

unshift is also pretty terribly slow. At this point, it'd make more sense to just flip it to a normal for loop and use push. There's no point in being clever and looping in reverse now if we acknowledge that there can be gaps in the array.

johnbacon · 2014-01-11T23:36:18Z

I have no problem moving to a normal for loop and away from unshift. As you stated, this seems to confirm there is still a significant performance difference between unshift and push.

The changes will be pushed again soon.

```unshift``` is significantly slower than ```push```. Shift towards using ```push``` and remove a redundant conditional.

@soundslocke

Likely [benefits](http://stackoverflow.com/questions/6261953/do-modern-javascript-jiters-need-array-length-caching-in-loops) the performance of older browsers that lack smart caching. Thanks again to @soundslocke for the heads up.

mattrobenolt · 2014-01-12T00:26:59Z

This looks sensible to me. Thanks for the contribution! 👍

I'll cut a new release today.

Improved solution to issue #179 (invalid options throwing errors in joinRegExp)

johnbacon · 2014-01-11T23:01:15Z

This slightly more robust solution ignores options that are not strings
or regular expressions in the passed patterns array.

The prior solution still accepted non-string, non-regexp options (e.g.
[], {}, true, etc.).

Credit goes to @soundslocke for the fix.

@soundslocke

…rors in joinRegExp) This slightly more robust solution ignores options that are not strings or regular expressions in the passed patterns array. The prior solution still accepted non-string, non-regexp options (e.g. [], {}, true, etc.). Credit goes to @soundslocke for the fix.

mattrobenolt · 2014-01-11T23:21:46Z

Is it necessary to check again for isUndefined? Wouldn't just patterns[i] && patterns[i].source be enough?

mattrobenolt · 2014-01-11T23:24:11Z

unshift is also pretty terribly slow. At this point, it'd make more sense to just flip it to a normal for loop and use push. There's no point in being clever and looping in reverse now if we acknowledge that there can be gaps in the array.

johnbacon · 2014-01-11T23:36:18Z

I have no problem moving to a normal for loop and away from unshift. As you stated, this seems to confirm there is still a significant performance difference between unshift and push.

The changes will be pushed again soon.

```unshift``` is significantly slower than ```push```. Shift towards using ```push``` and remove a redundant conditional.

@soundslocke

Likely [benefits](http://stackoverflow.com/questions/6261953/do-modern-javascript-jiters-need-array-length-caching-in-loops) the performance of older browsers that lack smart caching. Thanks again to @soundslocke for the heads up.

mattrobenolt · 2014-01-12T00:26:59Z

This looks sensible to me. Thanks for the contribution! 👍

I'll cut a new release today.

Improved solution to issue #179 (invalid options throwing errors in joinRegExp)

johnbacon · 2014-01-11T23:01:15Z

This slightly more robust solution ignores options that are not strings
or regular expressions in the passed patterns array.

The prior solution still accepted non-string, non-regexp options (e.g.
[], {}, true, etc.).

Credit goes to @soundslocke for the fix.

@soundslocke

…rors in joinRegExp) This slightly more robust solution ignores options that are not strings or regular expressions in the passed patterns array. The prior solution still accepted non-string, non-regexp options (e.g. [], {}, true, etc.). Credit goes to @soundslocke for the fix.

mattrobenolt · 2014-01-11T23:21:46Z

Is it necessary to check again for isUndefined? Wouldn't just patterns[i] && patterns[i].source be enough?

mattrobenolt · 2014-01-11T23:24:11Z

unshift is also pretty terribly slow. At this point, it'd make more sense to just flip it to a normal for loop and use push. There's no point in being clever and looping in reverse now if we acknowledge that there can be gaps in the array.

johnbacon · 2014-01-11T23:36:18Z

I have no problem moving to a normal for loop and away from unshift. As you stated, this seems to confirm there is still a significant performance difference between unshift and push.

The changes will be pushed again soon.

```unshift``` is significantly slower than ```push```. Shift towards using ```push``` and remove a redundant conditional.

@soundslocke

Likely [benefits](http://stackoverflow.com/questions/6261953/do-modern-javascript-jiters-need-array-length-caching-in-loops) the performance of older browsers that lack smart caching. Thanks again to @soundslocke for the heads up.

mattrobenolt · 2014-01-12T00:26:59Z

This looks sensible to me. Thanks for the contribution! 👍

I'll cut a new release today.

Improved solution to issue #179 (invalid options throwing errors in joinRegExp)

johnbacon · 2014-01-11T23:01:15Z

This slightly more robust solution ignores options that are not strings
or regular expressions in the passed patterns array.

The prior solution still accepted non-string, non-regexp options (e.g.
[], {}, true, etc.).

Credit goes to @soundslocke for the fix.

@soundslocke

…rors in joinRegExp) This slightly more robust solution ignores options that are not strings or regular expressions in the passed patterns array. The prior solution still accepted non-string, non-regexp options (e.g. [], {}, true, etc.). Credit goes to @soundslocke for the fix.

mattrobenolt · 2014-01-11T23:21:46Z

Is it necessary to check again for isUndefined? Wouldn't just patterns[i] && patterns[i].source be enough?

mattrobenolt · 2014-01-11T23:24:11Z

unshift is also pretty terribly slow. At this point, it'd make more sense to just flip it to a normal for loop and use push. There's no point in being clever and looping in reverse now if we acknowledge that there can be gaps in the array.

johnbacon · 2014-01-11T23:36:18Z

I have no problem moving to a normal for loop and away from unshift. As you stated, this seems to confirm there is still a significant performance difference between unshift and push.

The changes will be pushed again soon.

```unshift``` is significantly slower than ```push```. Shift towards using ```push``` and remove a redundant conditional.

@soundslocke

Likely [benefits](http://stackoverflow.com/questions/6261953/do-modern-javascript-jiters-need-array-length-caching-in-loops) the performance of older browsers that lack smart caching. Thanks again to @soundslocke for the heads up.

mattrobenolt · 2014-01-12T00:26:59Z

This looks sensible to me. Thanks for the contribution! 👍

I'll cut a new release today.

Improved solution to issue #179 (invalid options throwing errors in joinRegExp)

johnbacon · 2014-01-11T23:01:15Z

This slightly more robust solution ignores options that are not strings
or regular expressions in the passed patterns array.

The prior solution still accepted non-string, non-regexp options (e.g.
[], {}, true, etc.).

Credit goes to @soundslocke for the fix.

@soundslocke

…rors in joinRegExp) This slightly more robust solution ignores options that are not strings or regular expressions in the passed patterns array. The prior solution still accepted non-string, non-regexp options (e.g. [], {}, true, etc.). Credit goes to @soundslocke for the fix.

mattrobenolt · 2014-01-11T23:21:46Z

Is it necessary to check again for isUndefined? Wouldn't just patterns[i] && patterns[i].source be enough?

mattrobenolt · 2014-01-11T23:24:11Z

unshift is also pretty terribly slow. At this point, it'd make more sense to just flip it to a normal for loop and use push. There's no point in being clever and looping in reverse now if we acknowledge that there can be gaps in the array.

johnbacon · 2014-01-11T23:36:18Z

I have no problem moving to a normal for loop and away from unshift. As you stated, this seems to confirm there is still a significant performance difference between unshift and push.

The changes will be pushed again soon.

```unshift``` is significantly slower than ```push```. Shift towards using ```push``` and remove a redundant conditional.

@soundslocke

Likely [benefits](http://stackoverflow.com/questions/6261953/do-modern-javascript-jiters-need-array-length-caching-in-loops) the performance of older browsers that lack smart caching. Thanks again to @soundslocke for the heads up.

mattrobenolt · 2014-01-12T00:26:59Z

This looks sensible to me. Thanks for the contribution! 👍

I'll cut a new release today.

Improved solution to issue #179 (invalid options throwing errors in joinRegExp)

johnbacon · 2014-01-11T23:01:15Z

This slightly more robust solution ignores options that are not strings
or regular expressions in the passed patterns array.

The prior solution still accepted non-string, non-regexp options (e.g.
[], {}, true, etc.).

Credit goes to @soundslocke for the fix.

@soundslocke

…rors in joinRegExp) This slightly more robust solution ignores options that are not strings or regular expressions in the passed patterns array. The prior solution still accepted non-string, non-regexp options (e.g. [], {}, true, etc.). Credit goes to @soundslocke for the fix.

mattrobenolt · 2014-01-11T23:21:46Z

Is it necessary to check again for isUndefined? Wouldn't just patterns[i] && patterns[i].source be enough?

mattrobenolt · 2014-01-11T23:24:11Z

unshift is also pretty terribly slow. At this point, it'd make more sense to just flip it to a normal for loop and use push. There's no point in being clever and looping in reverse now if we acknowledge that there can be gaps in the array.

johnbacon · 2014-01-11T23:36:18Z

I have no problem moving to a normal for loop and away from unshift. As you stated, this seems to confirm there is still a significant performance difference between unshift and push.

The changes will be pushed again soon.

```unshift``` is significantly slower than ```push```. Shift towards using ```push``` and remove a redundant conditional.

@soundslocke

Likely [benefits](http://stackoverflow.com/questions/6261953/do-modern-javascript-jiters-need-array-length-caching-in-loops) the performance of older browsers that lack smart caching. Thanks again to @soundslocke for the heads up.

mattrobenolt · 2014-01-12T00:26:59Z

This looks sensible to me. Thanks for the contribution! 👍

I'll cut a new release today.

Improved solution to issue #179 (invalid options throwing errors in joinRegExp)

johnbacon · 2014-01-11T23:01:15Z

This slightly more robust solution ignores options that are not strings
or regular expressions in the passed patterns array.

The prior solution still accepted non-string, non-regexp options (e.g.
[], {}, true, etc.).

Credit goes to @soundslocke for the fix.

@soundslocke

…rors in joinRegExp) This slightly more robust solution ignores options that are not strings or regular expressions in the passed patterns array. The prior solution still accepted non-string, non-regexp options (e.g. [], {}, true, etc.). Credit goes to @soundslocke for the fix.

mattrobenolt · 2014-01-11T23:21:46Z

Is it necessary to check again for isUndefined? Wouldn't just patterns[i] && patterns[i].source be enough?

mattrobenolt · 2014-01-11T23:24:11Z

unshift is also pretty terribly slow. At this point, it'd make more sense to just flip it to a normal for loop and use push. There's no point in being clever and looping in reverse now if we acknowledge that there can be gaps in the array.

johnbacon · 2014-01-11T23:36:18Z

I have no problem moving to a normal for loop and away from unshift. As you stated, this seems to confirm there is still a significant performance difference between unshift and push.

The changes will be pushed again soon.

```unshift``` is significantly slower than ```push```. Shift towards using ```push``` and remove a redundant conditional.

@soundslocke

Likely [benefits](http://stackoverflow.com/questions/6261953/do-modern-javascript-jiters-need-array-length-caching-in-loops) the performance of older browsers that lack smart caching. Thanks again to @soundslocke for the heads up.

mattrobenolt · 2014-01-12T00:26:59Z

This looks sensible to me. Thanks for the contribution! 👍

I'll cut a new release today.

Improved solution to issue #179 (invalid options throwing errors in joinRegExp)

mattrobenolt reviewed Jan 11, 2014
View reviewed changes

johnbacon added 2 commits January 11, 2014 18:49

Refactor towards a for loop and .push while removing redundant check
1d91556
```unshift``` is significantly slower than ```push```. Shift towards using ```push``` and remove a redundant conditional.

Cache the value of patterns.length outside the for loop
74b868d
Likely [benefits](http://stackoverflow.com/questions/6261953/do-modern-javascript-jiters-need-array-length-caching-in-loops) the performance of older browsers that lack smart caching. Thanks again to @soundslocke for the heads up.

mattrobenolt added a commit that referenced this pull request Jan 12, 2014
Merge pull request#180from johnbacon/patch-1
f3baeee
Improved solution to issue #179 (invalid options throwing errors in joinRegExp)

mattrobenolt merged commit f3baeee into getsentry:masterJan 12, 2014

mattrobenolt reviewed Jan 11, 2014
View reviewed changes

johnbacon added 2 commits January 11, 2014 18:49

Refactor towards a for loop and .push while removing redundant check
1d91556
```unshift``` is significantly slower than ```push```. Shift towards using ```push``` and remove a redundant conditional.

Cache the value of patterns.length outside the for loop
74b868d
Likely [benefits](http://stackoverflow.com/questions/6261953/do-modern-javascript-jiters-need-array-length-caching-in-loops) the performance of older browsers that lack smart caching. Thanks again to @soundslocke for the heads up.

mattrobenolt added a commit that referenced this pull request Jan 12, 2014
Merge pull request#180from johnbacon/patch-1
f3baeee
Improved solution to issue #179 (invalid options throwing errors in joinRegExp)

mattrobenolt merged commit f3baeee into getsentry:masterJan 12, 2014

mattrobenolt reviewed Jan 11, 2014
View reviewed changes

johnbacon added 2 commits January 11, 2014 18:49

Refactor towards a for loop and .push while removing redundant check
1d91556
```unshift``` is significantly slower than ```push```. Shift towards using ```push``` and remove a redundant conditional.

Cache the value of patterns.length outside the for loop
74b868d
Likely [benefits](http://stackoverflow.com/questions/6261953/do-modern-javascript-jiters-need-array-length-caching-in-loops) the performance of older browsers that lack smart caching. Thanks again to @soundslocke for the heads up.

mattrobenolt added a commit that referenced this pull request Jan 12, 2014
Merge pull request#180from johnbacon/patch-1
f3baeee
Improved solution to issue #179 (invalid options throwing errors in joinRegExp)

mattrobenolt merged commit f3baeee into getsentry:masterJan 12, 2014

mattrobenolt reviewed Jan 11, 2014
View reviewed changes

johnbacon added 2 commits January 11, 2014 18:49

Refactor towards a for loop and .push while removing redundant check
1d91556
```unshift``` is significantly slower than ```push```. Shift towards using ```push``` and remove a redundant conditional.

Cache the value of patterns.length outside the for loop
74b868d
Likely [benefits](http://stackoverflow.com/questions/6261953/do-modern-javascript-jiters-need-array-length-caching-in-loops) the performance of older browsers that lack smart caching. Thanks again to @soundslocke for the heads up.

mattrobenolt added a commit that referenced this pull request Jan 12, 2014
Merge pull request#180from johnbacon/patch-1
f3baeee
Improved solution to issue #179 (invalid options throwing errors in joinRegExp)

mattrobenolt merged commit f3baeee into getsentry:masterJan 12, 2014

mattrobenolt reviewed Jan 11, 2014
View reviewed changes

johnbacon added 2 commits January 11, 2014 18:49

Refactor towards a for loop and .push while removing redundant check
1d91556
```unshift``` is significantly slower than ```push```. Shift towards using ```push``` and remove a redundant conditional.

Cache the value of patterns.length outside the for loop
74b868d
Likely [benefits](http://stackoverflow.com/questions/6261953/do-modern-javascript-jiters-need-array-length-caching-in-loops) the performance of older browsers that lack smart caching. Thanks again to @soundslocke for the heads up.

mattrobenolt added a commit that referenced this pull request Jan 12, 2014
Merge pull request#180from johnbacon/patch-1
f3baeee
Improved solution to issue #179 (invalid options throwing errors in joinRegExp)

mattrobenolt merged commit f3baeee into getsentry:masterJan 12, 2014

mattrobenolt reviewed Jan 11, 2014
View reviewed changes

johnbacon added 2 commits January 11, 2014 18:49

Refactor towards a for loop and .push while removing redundant check
1d91556
```unshift``` is significantly slower than ```push```. Shift towards using ```push``` and remove a redundant conditional.

Cache the value of patterns.length outside the for loop
74b868d
Likely [benefits](http://stackoverflow.com/questions/6261953/do-modern-javascript-jiters-need-array-length-caching-in-loops) the performance of older browsers that lack smart caching. Thanks again to @soundslocke for the heads up.

mattrobenolt added a commit that referenced this pull request Jan 12, 2014
Merge pull request#180from johnbacon/patch-1
f3baeee
Improved solution to issue #179 (invalid options throwing errors in joinRegExp)

mattrobenolt merged commit f3baeee into getsentry:masterJan 12, 2014

mattrobenolt reviewed Jan 11, 2014
View reviewed changes

johnbacon added 2 commits January 11, 2014 18:49

Refactor towards a for loop and .push while removing redundant check
1d91556
```unshift``` is significantly slower than ```push```. Shift towards using ```push``` and remove a redundant conditional.

Cache the value of patterns.length outside the for loop
74b868d
Likely [benefits](http://stackoverflow.com/questions/6261953/do-modern-javascript-jiters-need-array-length-caching-in-loops) the performance of older browsers that lack smart caching. Thanks again to @soundslocke for the heads up.

mattrobenolt added a commit that referenced this pull request Jan 12, 2014
Merge pull request#180from johnbacon/patch-1
f3baeee
Improved solution to issue #179 (invalid options throwing errors in joinRegExp)

mattrobenolt merged commit f3baeee into getsentry:masterJan 12, 2014

Uh oh!

Uh oh!

Conversation

johnbacon commented Jan 11, 2014

Uh oh!

mattrobenoltJan 11, 2014

Choose a reason for hiding this comment

Uh oh!

mattrobenolt commented Jan 11, 2014

Uh oh!

johnbacon commented Jan 11, 2014

Uh oh!

mattrobenolt commented Jan 12, 2014

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Uh oh!

Conversation

johnbacon commented Jan 11, 2014

Uh oh!

mattrobenoltJan 11, 2014

Choose a reason for hiding this comment

Uh oh!

mattrobenolt commented Jan 11, 2014

Uh oh!

johnbacon commented Jan 11, 2014

Uh oh!

mattrobenolt commented Jan 12, 2014

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Uh oh!

Conversation

johnbacon commented Jan 11, 2014

Uh oh!

mattrobenoltJan 11, 2014

Choose a reason for hiding this comment

Uh oh!

mattrobenolt commented Jan 11, 2014

Uh oh!

johnbacon commented Jan 11, 2014

Uh oh!

mattrobenolt commented Jan 12, 2014

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Uh oh!

Conversation

johnbacon commented Jan 11, 2014

Uh oh!

mattrobenoltJan 11, 2014

Choose a reason for hiding this comment

Uh oh!

mattrobenolt commented Jan 11, 2014

Uh oh!

johnbacon commented Jan 11, 2014

Uh oh!

mattrobenolt commented Jan 12, 2014

Uh oh!