americanexpress / jest-image-snapshot

✨ Jest matcher for image comparisons. Most commonly used for visual regression testing.
Apache License 2.0
3.81k stars 197 forks source link

Feat(perf): replace pngjs with node-libpng #303

Closed acb122 closed 1 year ago

acb122 commented 1 year ago

I would like to propose removing pngjs for node-libpng which is a native library allowing for much improved performance.

Description

Remove the uses of pngjs for node-libpng. It is much faster. It would also allow to remove the spawn new process code as the speed is now equivelent with using a new process making the code much simpilier and easier to debug.

Motivation and Context

Increase performace Simplify code

How Has This Been Tested?

All tests are passing, just one image needed replacing cause it is checked on the encoded level

Types of Changes

Checklist:

What is the Impact to Developers Using Jest-Image-Snapshot?

Quicker tests

aloisklink commented 1 year ago

I'm not a member of @americanexpress, or a contributor to this repo, but I'm against this.

node-libpng currently only supports Node <=16 Linux/Windows/MacOS x86 64-bit (see https://github.com/Prior99/node-libpng#supported-environments)

There's no support for Node v18, or for ARM based systems, which seems pretty important, since new Macs all have ARM processors. It's up to the maintainers on whether that's worth the speedup that node-libpng might bring.

10xLaCroixDrinker commented 1 year ago

Thanks for calling that out @aloisklink.

@acb122 with that in mind, we'll be closing this.

aloisklink commented 1 year ago

If node-libpng does ever add Node v18 and ARM support and you want to re-open this PR, it's also worth adding some basic benchmarks.

Even though node-libpng is much faster than pngjs, if jest-image-snapshot only spends 1% of the time in PNG parsing, the actual speedup for this project might be tiny, and not worth any future incompatibility issues.