fix: avoid throw for invalid cases when not needed

[H4ad]

Hey, how you doing?

I notice some cases where we don't want to throw when instantiating the semver, for those cases, I added a property to semver instead of throwing an error since this is an expensive operation on node.

Old for bench-parse:

inc(1.0.0) x 225,005 ops/sec ±2.02% (73 runs sampled)
inc(2.1.0) x 207,920 ops/sec ±0.69% (98 runs sampled)
inc(3.2.1) x 204,846 ops/sec ±1.11% (96 runs sampled)
inc(v1.2.3) x 179,107 ops/sec ±0.76% (95 runs sampled)
inc(1.2.3-0) x 2,507,827 ops/sec ±0.63% (95 runs sampled)
inc(1.2.3-123) x 2,040,282 ops/sec ±0.29% (96 runs sampled)
inc(1.2.3-1.2.3) x 1,632,962 ops/sec ±0.26% (93 runs sampled)
inc(1.2.3-1a) x 2,243,061 ops/sec ±0.19% (96 runs sampled)
inc(1.2.3-a1) x 2,263,182 ops/sec ±0.20% (97 runs sampled)
inc(1.2.3-alpha) x 2,216,367 ops/sec ±0.21% (96 runs sampled)
inc(1.2.3-alpha.1) x 1,767,612 ops/sec ±0.29% (98 runs sampled)
inc(1.2.3-alpha-1) x 2,175,641 ops/sec ±0.25% (95 runs sampled)
inc(1.2.3-alpha-.-beta) x 1,689,913 ops/sec ±0.22% (97 runs sampled)
inc(1.2.3+456) x 140,200 ops/sec ±0.56% (93 runs sampled)
inc(1.2.3+build) x 139,831 ops/sec ±0.47% (97 runs sampled)
inc(1.2.3+new-build) x 139,792 ops/sec ±0.44% (99 runs sampled)
inc(1.2.3+build.1) x 139,318 ops/sec ±0.39% (99 runs sampled)
inc(1.2.3+build.1a) x 138,640 ops/sec ±0.48% (98 runs sampled)
inc(1.2.3+build.a1) x 138,540 ops/sec ±0.39% (97 runs sampled)
inc(1.2.3+build.alpha) x 138,011 ops/sec ±0.44% (98 runs sampled)
inc(1.2.3+build.alpha.beta) x 136,775 ops/sec ±0.53% (95 runs sampled)
inc(1.2.3-alpha+build) x 1,700,055 ops/sec ±0.12% (99 runs sampled)
invalid inc(111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111.0.0) x 176,696 ops/sec ±0.68% (94 runs sampled)
invalid inc(90071992547409910.0.0) x 145,420 ops/sec ±0.56% (96 runs sampled)
invalid inc(0.90071992547409910.0) x 141,811 ops/sec ±0.39% (100 runs sampled)
invalid inc(0.0.90071992547409910) x 136,328 ops/sec ±0.61% (92 runs sampled)
invalid inc(hello, world) x 158,965 ops/sec ±0.62% (95 runs sampled)
invalid inc(hello, world) x 159,619 ops/sec ±0.59% (96 runs sampled)
invalid inc(xyz) x 159,924 ops/sec ±0.45% (100 runs sampled)
invalid inc(/a regexp/) x 174,861 ops/sec ±0.45% (92 runs sampled)
invalid inc(/1.2.3/) x 170,926 ops/sec ±0.56% (93 runs sampled)
invalid inc(1.2.3) x 174,531 ops/sec ±0.53% (97 runs sampled)

Now:

inc(1.0.0) x 220,004 ops/sec ±2.08% (71 runs sampled)
inc(2.1.0) x 203,418 ops/sec ±0.65% (96 runs sampled)
inc(3.2.1) x 204,638 ops/sec ±0.84% (99 runs sampled)
inc(v1.2.3) x 183,902 ops/sec ±0.57% (96 runs sampled)
inc(1.2.3-0) x 2,355,120 ops/sec ±0.32% (92 runs sampled)
inc(1.2.3-123) x 1,892,798 ops/sec ±0.26% (94 runs sampled)
inc(1.2.3-1.2.3) x 1,547,679 ops/sec ±0.31% (98 runs sampled)
inc(1.2.3-1a) x 2,177,573 ops/sec ±0.29% (98 runs sampled)
inc(1.2.3-a1) x 2,216,342 ops/sec ±0.18% (98 runs sampled)
inc(1.2.3-alpha) x 2,148,186 ops/sec ±0.30% (94 runs sampled)
inc(1.2.3-alpha.1) x 1,641,404 ops/sec ±0.19% (95 runs sampled)
inc(1.2.3-alpha-1) x 2,153,099 ops/sec ±0.19% (96 runs sampled)
inc(1.2.3-alpha-.-beta) x 1,651,880 ops/sec ±0.22% (98 runs sampled)
inc(1.2.3+456) x 143,325 ops/sec ±0.42% (99 runs sampled)
inc(1.2.3+build) x 142,020 ops/sec ±0.53% (91 runs sampled)
inc(1.2.3+new-build) x 141,831 ops/sec ±0.41% (98 runs sampled)
inc(1.2.3+build.1) x 141,295 ops/sec ±0.42% (98 runs sampled)
inc(1.2.3+build.1a) x 141,597 ops/sec ±0.46% (96 runs sampled)
inc(1.2.3+build.a1) x 141,936 ops/sec ±0.40% (98 runs sampled)
inc(1.2.3+build.alpha) x 141,680 ops/sec ±0.44% (97 runs sampled)
inc(1.2.3+build.alpha.beta) x 141,003 ops/sec ±0.41% (100 runs sampled)
inc(1.2.3-alpha+build) x 1,657,613 ops/sec ±0.17% (96 runs sampled)
invalid inc(111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111.0.0) x 33,693,858 ops/sec ±1.06% (91 runs sampled)
invalid inc(90071992547409910.0.0) x 4,140,107 ops/sec ±0.34% (93 runs sampled)
invalid inc(0.90071992547409910.0) x 4,202,449 ops/sec ±0.54% (96 runs sampled)
invalid inc(0.0.90071992547409910) x 4,074,528 ops/sec ±0.37% (94 runs sampled)
invalid inc(hello, world) x 16,226,916 ops/sec ±0.62% (92 runs sampled)
invalid inc(hello, world) x 16,308,847 ops/sec ±0.56% (98 runs sampled)
invalid inc(xyz) x 16,368,938 ops/sec ±0.58% (97 runs sampled)
invalid inc(/a regexp/) x 26,658,694 ops/sec ±0.58% (96 runs sampled)
invalid inc(/1.2.3/) x 26,474,634 ops/sec ±0.73% (97 runs sampled)
invalid inc(1.2.3) x 26,354,579 ops/sec ±0.82% (96 runs sampled)

I'm not sure if this is something you guys are willing to do (adding a new property and have the object partially dead), so l will keep this as draft for now.

[wraithgar]

Should throwErrors be in options?

[H4ad]

I didn't want to include since this would require me to modify/copy the options object to avoid mutate the original options just to include this flag.

[wraithgar]

I'm extremely hesitant to add Yet Another Parameter especially in an environment where we already use a kwargs-style options.

[H4ad]

It's ok to mutate the original options object? If so, I can move to the options object.

[wraithgar]

I don't think so. Sorry it's a busy week and I'm not really able to intuit why you'd need to mutate the options here. You can try to explain it or take a peek at internal/parse-options.js to see how we set defaults (i.e. loose=true).

[wraithgar]

Ok right now I'm remembering a bit of this, it's a bit of a funky parser cause what we're supporting is parse('1.2.3', true) which is kind of a shorthand in semver that the opts can just be defining loose. It ... was definitely a choice.

I think we can come up with a sensible default here, and it's worth working through so we're not adding a new parameter. We'd really like to encourage just passing an object here.

[H4ad]

I updated the benchmark and moved the noThrow option to the options object.

[H4ad]

To be honest, I saw I little perf hit due to the object spread on parse/inc to add the new property but the invalid cases where options by a lot.

So, the question is, how often do we have bad versions?

If you don't think is often, I don't think we should merge this PR with the noThrow in the options via spread.

[wraithgar]

So, the question is, how often do we have bad versions?

The use case for bad versions is definitely "before it hits a registry", so local package.json files. That's the furthest from a hot path possible. I think the optimization here should be for good data.

[H4ad]

Yes, that's what I thought. So, to be able to implement this change without any slowdowns in the hot path, it would be necessary to add a third parameter on Semver.

Do you think it's worth it? If not, I'll close this PR.

[wraithgar]

At this point I don't think adding a third parameter is the right way to go, sorry. I do appreciate the work put into this still.