[Break]: Golfing w/ Performance tuning

[lukeed]

This PR adds a few things at once, sorry.

Breaking

• Changed the 2nd param to be a boolean, toggling long-mode directly Of course this can be reverted, and I'm open to it, but don't foresee other options :)

Features

• Multiple formats:

  • ESM (via module entry)
  • CommonJS (via main entry – unchanged)
  • UMD (via unpkg entry)

• Made available on unpkg.com 🎉

Chores

• Adds minimal benchmark suite
• Attaches a light build tool (one of my own) to auto-build ^those formats
• Includes prettier directly – was relying on global installation
• Updates lint script

---

Here's a before-after of both the module size & the performance:

Running Node v10.13.0, no warmups

Before:

Filename                 Filesize  (gzip)
dist/index.js             1.41 kB  643 B
dist/index.mjs            1.41 kB  633 B
dist/index.min.js         1.56 kB  701 B


# string (short) -> number
  ms   x 11,828,412 ops/sec ±0.46% (92 runs sampled)

# string (long) -> number
  ms   x 5,803,783 ops/sec ±1.78% (96 runs sampled)

# number -> string (short)
  ms   x 76,444,334 ops/sec ±0.89% (96 runs sampled)

# number -> string (long)
  ms   x 33,576,081 ops/sec ±0.69% (93 runs sampled)

# Negative :: string (short) -> number
  ms   x 6,710,932 ops/sec ±1.41% (96 runs sampled)

# Negative :: string (long) -> number
  ms   x 5,944,477 ops/sec ±0.51% (95 runs sampled)

# Negative :: number -> string (short)
  ms   x 80,648,073 ops/sec ±0.22% (96 runs sampled)

# Negative :: number -> string (long)
  ms   x 32,387,825 ops/sec ±0.32% (98 runs sampled)

After

Filename                 Filesize  (gzip)
dist/index.js              948 B   521 B
dist/index.mjs             947 B   521 B
dist/index.min.js         1.10 kB  585 B


# string (short) -> number
  ms   x 13,121,298 ops/sec ±0.89% (93 runs sampled)

# string (long) -> number
  ms   x 6,087,008 ops/sec ±1.90% (93 runs sampled)

# number -> string (short)
  ms   x 77,564,203 ops/sec ±0.60% (91 runs sampled)

# number -> string (long)
  ms   x 46,140,236 ops/sec ±0.29% (93 runs sampled)

# Negative :: string (short) -> number
  ms   x 7,159,583 ops/sec ±0.30% (96 runs sampled)

# Negative :: string (long) -> number
  ms   x 6,152,167 ops/sec ±0.67% (94 runs sampled)

# Negative :: number -> string (short)
  ms   x 78,520,060 ops/sec ±0.64% (96 runs sampled)

# Negative :: number -> string (long)
  ms   x 45,736,035 ops/sec ±0.36% (95 runs sampled)

I have a few other versions locally, ranging from 569B to 487B, but they had different performance trade-offs. This 521B version feels like the best balance IMO.

[lukeed]

Any feedback on this? Been a little while :)

[leerob]

Hey! Thank you for the exploration here. Very interesting!

In the interest of ensuring this package is maintainable, readable, and most importantly stable for the millions who depend on it, we're going to be very selective of the PRs that get merged in.

Closing this as won't fix, but thank you regardless 🙏

[maraisr]

I don't know about that @leerob I'd argue this PR address some pretty serious things—stuff I believe should have been merged.

Few points;

• You mentioned maintainability now having benchmarks means this can be better maintained knowing there is no regression on performance.
• You mentioned readability. Having a variables named n is far less readable than that of num. But as this is biased, could you work with @lukeed to align some of this concerns?
• Stability, I'd argue this is more stable than whats out there now, as this has tests and benchmark to prove it.

@leerob I urge you to reconsider this one.

[leerob]

You mentioned maintainability now having benchmarks means this can be better maintained knowing there is no regression on performance.

We will likely not be merging large feature requests at all here, only bug fixes or security vulnerabilities (in my opinion). This library does one thing, and one thing well. I don't believe it's in its ethos to expand further when millions depend on it doing that one thing.

Stability, I'd argue this is more stable than what's out there now, as this has tests and benchmark to prove it.

There are tests. On stability: if it's not broken, I don't see a need to introduce risk in merging something new.

[lukeed]

As mentioned at the top of the PR:

Breaking

Changed the 2nd param to be a boolean, toggling long-mode directly
Of course this can be reverted, and I'm open to it, but don't foresee other options :)

The breaking change here can easily be reverted, and you maintain the size reduction & performance win. Even if it weren't, proper versioning allows for there to be breaking changes without affecting existing users.

But I get it – this was just a nice way to express that there's no interest :) no problem

[leerob]

First off, I apologize for making you wait years to hear back from anyone 😅 There are some still interested in this, which I wasn't sure about due to lack of interest in the PR or related issue. I've had a few folks reach out and ask.

I'm happy to open this back up and create an issue, where we can track community interest for this PR. I agree about versioning. I'd say the first iteration of these changes would be without the breaking change, and then a separate PR could introduce that change. What are your thoughts on that?

[lukeed]

Hmm.. so this is a 180 from your previous comments, wherein you said this was a "won't fix" and you ❤️'d my "you're just being nice about having no interest" comment, haha

Your suggestion is exactly what I proposed at the top. And I pointed this out again in my recent comment -- the one you ❤️'d -- so I took that to mean you (maybe collectively?) were still not interested in this. The time & lack of response, of course, furthers this impression.

It seems to me that the sudden turnaround is only because I published my fork...

[leerob]

I left a heart because I appreciate you being honest and civil. It's easy to lose the meaning behind a comment, especially when you don't know me at all. So thank you for being honest and open!

And yes, I am open to change based on your comment and @maraisr's 😄 I'll be honest as well, there wasn't a collective decision on this. It's mostly myself trying to clean-up random depths of the Vercel GitHub org. So I'm probably just wrong, especially your comment about versioning.

You're free to continue forking as well. I will let you decide how you'd like to move this forward.

[lukeed]

Gotcha. Well, I can restore the branch & you could reopen the PR, but TBH I'm kinda done waiting & putting effort into this contribution, so you/someone else is going to have to put in the extra bit of elbow grease to resolve conflicts & revert the parameter change. It's been loud & clear that it wasn't/never was wanted, and that has turned me off on investing anything else here.

I'll keep my fork – only because I've already been using it for a year & prefer its API. But again, I'm more than happy for this PR's size & perf improvements to make its way into ms because it'll help out more people. My fork will be very insignificant, at best/if anything.

Even though you ultimately 180'd, I appreciate that you took the time to say something @leerob :)

[styfle]

I think the reason this PR was never merged is because its doing too many things.

I would accept a PR doing these things:

• Adds minimal benchmark suite
• Includes prettier directly

[lukeed]

I'm sorry – I generally agree with small PRs, but I really don't think that was the case here:

• there has to be a benchmark to prove the performance improvements
• I had to include/update prettier to actually get the PR in a passing state, since it relied on global installation

At best, the "multiple formats" feature could have been a separate PR, but it was included here to track/prove the size reduction.

Either way, I would have been happy to address any of this between June 2019-2020 had it been raised. I know it's nothing to do with you personally @styfle (or @leerob) – nor is it your fault(s) – but overall this could have been handled much better at any point in time.

TBH I'm still not sure if this (or parts of this) is actually wanted, or if the conversation is only continuing because my fork is public/published, which gave this thread some visibility (sorry for that btw, not intended... just wanted to give proper credit & reference as to why my fork exists). Until I know that answer, I'm not inclined to invest any more time/focus here, but as stated above, I'd still like to make the contribution.

I'm not insisting that you accept the PR by any means. A rejection is perfectly acceptable – even an immediate rejection would have been okay! It's your project & I'll accept whatever the decision may be. It's just that after a long wait period, I'm left confused w/ mixed signals.

[Nytelife26]

I'm not sure how people intend to move this forward, but there is definitely interest. @lukeed, computer science is about science. I'm sorry you had to deal with the whole "if it isn't broken don't fix it" ideology.

If you do not intend to continue working on this, I'm happy to pick it up.

[abzr1]

@Nytelife26 he doesn't intend to continue working on it. Instead, he has published his own fork: lukeed/ms.