Convert from Express to tinyhttp

[groenroos]

Spike PR to explore converting from Express to tinyhttp, to see how difficult it is, and what kind of performance boost it may provide.

Closes #97.

Notes

• Chose to rename the imported App to TinyHTTP in order to avoid wider confusion with the pre-existing Sapling App object.
• tinyhttp doesn't do static assets out of the box, this requires a separate dependency; went with sirv, seems to be a seamless drop-in replacement
• The missing 9% of code coverage accounts for some very key parts of the app, so some manual testing will be required

[groenroos]

@talentlessguy It seems like the switch was generally less painful than I was expecting!

However, one problem that fails the test suite and makes this unmergeable for the moment is that tinyhttp doesn't seem to play well with supertest (or vice versa, I suppose).

It appears that supertest expects the Express instance that is passed to it to have a method called address, and cannot cope without it;

Error thrown in test:

TypeError {
    message: 'app.address is not a function',
}

› Test.serverAddress (node_modules/supertest/lib/test.js:57:18)
› new Test (node_modules/supertest/lib/test.js:38:12)
› Object.obj.<computed> (node_modules/supertest/index.js:27:14)
› test/core/loadRest.test.js:76:10

Any idea how to circumnavigate this?

[v1rtl]

the easiest solution is this:

const server = app.listen()

const request = supertest(server)

tinyhttp has such thing in its own tests

[groenroos]

Seems like that did do the trick, thank you! 🎉

The final failing test has to do with csurf; the pattern for custom responses in CSRF token failures according to their README is as it's implemented below:

sapling/core/loadServer.js

Lines 100 to 106 in c4f2c72

	server.use((error, request, response, next) => {
	if (error.code !== 'EBADCSRFTOKEN') {
	return next(error);
	}
	new Response(this, request, response, new SaplingError('Invalid CSRF token'));
	});

However, tinyhttp's first callback parameter isn't error, so this pattern doesn't work. I also see that the token error is just thrown into the console.

Is there an established pattern on how to deal with this case?

[groenroos]

Alright, looks like I finally got the tinyhttp implementation to a stable state. Overall, it was easier than I expected, and the app still works as before.

One possible future gotcha is around the csurf error handling, in that if other functionality needs to catch errors in the future, they're going to overwrite each others' error handlers. It's not a problem for now, though.

Having done a few unscientific benchmarks, looks like Sapling with tinyhttp is about 36% faster than with Express:

tinyhttp

Running 30s test @ http://localhost:4000
  12 threads and 400 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency    74.99ms    6.96ms 160.76ms   93.18%
    Req/Sec   266.75    142.11   575.00     58.62%
  95616 requests in 30.09s, 1.02GB read
  Socket errors: connect 157, read 185, write 0, timeout 0
Requests/sec:   3177.22
Transfer/sec:     34.60MB

Express

Running 30s test @ http://localhost:4000
  12 threads and 400 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency   101.92ms   13.54ms 249.82ms   94.62%
    Req/Sec   196.95    102.21   535.00     58.23%
  70262 requests in 30.09s, 765.87MB read
  Socket errors: connect 157, read 193, write 0, timeout 0
Requests/sec:   2334.68
Transfer/sec:     25.45MB

[groenroos]

Overall, looks good! However, before merging this, I think I want to complete #126, #129 and #130 first - ticking off the last bits of missing code coverage should make it easier to ensure this version is fully equivalent.

[codecov]

Codecov Report

Merging #200 (28c0f2e) into master (c49d115) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 28c0f2e differs from pull request most recent head cb5c484. Consider uploading reports for the commit cb5c484 to get more accurate results

@@           Coverage Diff           @@
##           master     #200   +/-   ##
=======================================
  Coverage   98.21%   98.21%           
=======================================
  Files          36       36           
  Lines        4415     4416    +1     
=======================================
+ Hits         4336     4337    +1     
  Misses         79       79           

Impacted Files	Coverage Δ
core/loadServer.js	94.52% <100.00%> (+0.03%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c49d115...cb5c484. Read the comment docs.

[groenroos]

Tests written and passing, merge conflicts solved, and even the lockfile finally agreed to work! Happy to merge this and enjoy the 36% performance improvement!