snyk ms:20170412 performance optimization

[Delagen]

• dramatically performance optimization
• added test for Infinity and long strings performance
• standard style fix, fix missing devDependencies

[leo]

Looks good to me! But can you please remove standard and replace it with xo? Or simply leave it out and we'll add it later.

[Delagen]

What you don't like in standard? May be left only eslint with fixers!?

[leo]

Because we have XO on almost all of our repositories... 😊

[Delagen]

replaced by eslint-config-xo which IMHO better than second linter. XO is simply eslint wrapper, so I think using base module is more preferred

[Delagen]

Any review?

[leo]

Will have a look soon! Currently pretty busy.

[thevtm]

Hi!
I did a little benchmarking and found out that the original implementation seems to be around 2x faster than the PR implementation.

Benchmark source

original x 350,875 ops/sec ±1.68% (84 runs sampled)
new x 130,267 ops/sec ±1.02% (87 runs sampled)
Fastest is original
Done in 12.50s.

[Delagen]

@thevtm Just insert one test value '1'.repeat(29) + 'Z' to invert results. I agree that on small correct values original is faster, but performance must be predictable. This PR mainly resolve issue with ReDoS and removes limit of 100 chars as workaround for this security issue, that still exists

[thevtm]

@Delagen thanks! I haven't considered that case.
I think I found a better solution.
Changing the Regex from:
/^((?:\d+)?\.?\d+) *(milliseconds?|msecs?|ms|seconds?|secs?|s|minutes?|mins?|m|hours?|hrs?|h|days?|d|years?|yrs?|y)?$/i
to:
/^((?:\d+)?\.?\d+) *(\w{0,12})?$/i
seems to produce better results in all cases.

Benchmark source

original x 38,472 ops/sec ±2.05% (88 runs sampled)
new x 39,713 ops/sec ±1.89% (82 runs sampled)
mine x 74,820 ops/sec ±1.84% (84 runs sampled)
Fastest is mine
Done in 17.97s.

[Delagen]

@thevtm thanks for benchmarking, updated code with your regexp

[knoxcard]

You guys are amazing, thanks, look forward to pulling the latest!

[TooTallNate]

This module is meant to be run in a variety of JavaScript environments, including (possibly older) web browsers. I don't think we'll be able to use Map here.

[Delagen]

@TooTallNate if you about IE8, It's dead, even webpack removed support of it

[ForbesLindesay]

@Delagen Even IE10 doesn't support Map. IE11's support is pretty hit and miss (although it probably supports everything you need here): https://kangax.github.io/compat-table/es6/

[Delagen]

@ForbesLindesay Modern development use a couple bunch of polyfills or transpilers when target IE, so it's not a problem at all. This PR is about 1 year old, so it can never merge.
Moreover IE11 support get/set methods, but not support constructor params, so it can rewritten to simple loop. But IE10 need polyfill

[rauchg]

Interested in this, but PR has to be re-factored