Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: loupe@2 #14

Merged
merged 23 commits into from
Nov 15, 2019
Merged

feat: loupe@2 #14

merged 23 commits into from
Nov 15, 2019

Conversation

keithamus
Copy link
Member

This is a rewrite of loupe from the ground up.

It uses much better truncation than the existing algo which will be very handy for new error messages. It also supports all new ES types including Map/Set/Promise, as well as printing enumerable properties and more.

@keithamus keithamus mentioned this pull request Jan 2, 2019
@keithamus
Copy link
Member Author

@meeber @lucasfcosta whenever you have time a review of this code would be good. It's about 90% of where I want it to be. It needs better test coverage, and I'd like a few more additions before we release @2 - chiefly DOM support. However I'm happy enough with the implementation to start scrutinising it!

@keithamus keithamus requested a review from a team January 2, 2019 10:27
Copy link
Member

@lucasfcosta lucasfcosta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really nice!

Good work, @keithamus. I've done a first pass on it and didn't really find anything big, perhaps the most important thing would be to always consider customInspect first.

Other things are mostly syntax tips. I won't mark as approve or disapprove because I don't know how important everyone will consider those things to be.

bench/index.js Show resolved Hide resolved
const type = typeDetect(value)

// If it is a base value that we already support, then use Loupe's inspector
if (baseTypesMap[type]) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't someone set a custom inspect for those base values? IMO it's not an exotic use case and therefore the check for customInspectcould come first. Not sure I'm missing something here though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea here is to keep the base types protected. We don't want users to override base types to provide their own inspection; the custom inspection is really meant as a fallback for users who want to provide inspection for their own objects, not for builtins.

Essentially, I want to discourage the use of overriding for builtins because I don't think it will add value to this lib. Happy to hear otherwise though.

lib/array.js Outdated Show resolved Hide resolved
lib/error.js Outdated Show resolved Hide resolved
Copy link

@meeber meeber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love it

index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
lucasfcosta and others added 16 commits January 22, 2019 17:19
Co-Authored-By: keithamus <keithamus@users.noreply.github.com>
An attempt to simplify can be made later
These aren't needed as every call to inspect effectively does this
within `normaliseOptions`
We can call `Object.keys(a).slice(a.length)` which is must faster than
`getOwnPropertyNames` and does the same thing for our purposes. It does
not show non-enumerable keys but that is very much an edge case which
we're not interested in.

We can also return early if nonIndexProperties is empty as well as the
array

perf before this change:

array literal       (loupe) x 400,761 ops/sec ±0.68% (91 runs sampled)
array literal        (node) x 487,434 ops/sec ±0.74% (93 runs sampled)

perf after this change:

array literal       (loupe) x 766,210 ops/sec ±1.19% (87 runs sampled)
array literal        (node) x 474,837 ops/sec ±1.30% (88 runs sampled)
@keithamus keithamus mentioned this pull request Apr 26, 2019
@keithamus
Copy link
Member Author

I just pushed a few more commits. I'd say this is now about 95% of the way there. I'd like more acceptance tests that inspect custom objects, so we can see if they do the reasonable thing; I'd also like to find examples of tests where people have been unsatisfied with unclear error messages (like expected `{}` to equal `{}`) so we can bring those into the acceptance test suite and improve on them.

Im happy with all the benchmarks, it seems to match or exceed perf (by sometimes large margins) against Node's built in inspect, which I'm very happy with. I'd like to also see more complex objects be added to our benchmark suite so we can have a better (maybe fairer) comparison.

Benchmark results on my machine
string literal      (loupe) x 10,708,898 ops/sec ±2.78% (84 runs sampled)
string literal       (node) x 1,286,631 ops/sec ±0.82% (91 runs sampled)
array literal       (loupe) x 767,567 ops/sec ±0.93% (90 runs sampled)
array literal        (node) x 415,834 ops/sec ±1.02% (88 runs sampled)
boolean literal     (loupe) x 5,357,997 ops/sec ±15.21% (60 runs sampled)
boolean literal      (node) x 1,357,728 ops/sec ±6.72% (70 runs sampled)
object literal      (loupe) x 1,120,755 ops/sec ±4.43% (85 runs sampled)
object literal       (node) x 481,881 ops/sec ±3.60% (86 runs sampled)
object from null    (loupe) x 2,863,775 ops/sec ±2.85% (85 runs sampled)
object from null     (node) x 485,063 ops/sec ±1.70% (88 runs sampled)
regex literal       (loupe) x 4,778,276 ops/sec ±0.93% (89 runs sampled)
regex literal        (node) x 491,643 ops/sec ±0.90% (85 runs sampled)
number literal      (loupe) x 8,288,681 ops/sec ±0.96% (91 runs sampled)
number literal       (node) x 1,020,729 ops/sec ±9.90% (43 runs sampled)
null                (loupe) x 9,285,955 ops/sec ±6.18% (73 runs sampled)
null                 (node) x 1,642,534 ops/sec ±3.67% (85 runs sampled)
undefined           (loupe) x 8,703,288 ops/sec ±2.45% (87 runs sampled)
undefined            (node) x 1,668,263 ops/sec ±2.02% (89 runs sampled)
buffer              (loupe) x 1,186,755 ops/sec ±1.54% (86 runs sampled)
buffer               (node) x 320,706 ops/sec ±0.93% (88 runs sampled)
date                (loupe) x 689,241 ops/sec ±1.14% (89 runs sampled)
date                 (node) x 159,165 ops/sec ±13.81% (69 runs sampled)
map                 (loupe) x 8,680,445 ops/sec ±16.39% (73 runs sampled)
map                  (node) x 455,765 ops/sec ±6.58% (82 runs sampled)
map (complex)       (loupe) x 8,259,118 ops/sec ±4.02% (85 runs sampled)
map (complex)        (node) x 281,682 ops/sec ±3.21% (87 runs sampled)
regex constructor   (loupe) x 4,363,195 ops/sec ±2.73% (88 runs sampled)
regex constructor    (node) x 481,065 ops/sec ±0.83% (88 runs sampled)
set                 (loupe) x 1,558,024 ops/sec ±0.74% (89 runs sampled)
set                  (node) x 475,525 ops/sec ±0.75% (87 runs sampled)
string constructor  (loupe) x 804,621 ops/sec ±12.60% (48 runs sampled)
string constructor   (node) x 498,479 ops/sec ±6.85% (71 runs sampled)
arguments           (loupe) x 743,980 ops/sec ±3.31% (83 runs sampled)
arguments            (node) x 295,000 ops/sec ±3.30% (88 runs sampled)
class               (loupe) x 1,384,042 ops/sec ±2.18% (91 runs sampled)
class                (node) x 627,095 ops/sec ±1.39% (88 runs sampled)
~Fin~

@keithamus
Copy link
Member Author

We've sat on this for a long time, and this is mostly my fault for trying to get everything perfect in a single PR. I'm going to merge this PR now, and continue to iterate on it to get it into a shape ready to release.

@keithamus keithamus merged commit 2e67501 into master Nov 15, 2019
@keithamus keithamus deleted the v2 branch November 15, 2019 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants