Add @polka/compression package #148
Conversation
|
Legend, thank you sir! I'll pass through tomorrow for the failing Node 8.x suite and do some docs shuffling. Looks great though 🙌 |
|
Ah so the failing suite is because on Node < 12.7, this package automatically disables brotli (since its unsupported). I just updated the tests to skip the brotli suite for node versions without it, and relaxed to |
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## next #148 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 4 5 +1
Lines 117 322 +205
==========================================
+ Hits 117 322 +205 ☔ View full report in Codecov by Sentry. |
|
Can probably get coverage back up by doing a test that uses |
|
I broke CI somehow - tests should be passing on 8/10/12 though (see https://github.com/lukeed/polka/actions/runs/254918502). |
|
After reading through open PR's I can see you're most likely very busy, just wanted to see if there was a chance at some point to merge @developit's work @lukeed? |
|
IIRC Luke found and fixed a bug in the version of this that we have inlined in WMR, I need to backport that here. |
|
Sorry for bumping this @lukeed @developit, but FYI Vite is also going to inline this PR (once we merge vitejs/vite#6557). If there is a chance to get it merged at one point, we could reduce the maintenance burden for both WMR and Vite. |
|
Is this ready to be merged? |
|
what a good PR 👏 @lukeed could be nice if you can take a look and move this forward |
|
While I like the implementation I wonder if such should be part of an application server or better be left to reverse proxies like nginx. For me applications servers only handle the application and not logging, tracing, auditing, caching or compression. |
|
@pke There are numerous cases where compression is necessary in an application server. Nobody should have to run a reverse proxy just for local development (as Patak alluded to above). |
|
I never needed compression during local development, what would be some of the use cases? I try to understand. Beside that I am all for merging this, but the project seems to be pretty much dead and unmaintained. I might switch back to koa, although its not express middleware compatible. |
|
Stable and dead are different things. There are two flavors of Polka and both work well for different use cases. This PR still needs me to backport some fixes I/we found in the wmr integration & tests to solidify those fixes long term. |
|
Nice to see you here @lukeed ;) I just thought a 2 year old PR, which is green is not merged seemed like a dead project. But I didn't understand the implications of this PRs merge. There are also a bunch of issues open without any response (incl one of me ;) so if there is anything we could help with to get "next" out of beta I'd be glad to assist. |
|
The implementation code of this PR is really appreciated, there is no real alternatives these days for taking advantage of brotli, so I decided to just put it in standalone package and anyone can take advantage of it https://github.com/Kikobeats/http-compression Full credits to @developit for writing that simple and elegant code, I promise to maintain the code and keep it evolving |
packages/compression/index.js
Outdated
| const listeners = pendingListeners; | ||
| pendingListeners = null; | ||
| listeners.forEach(p => on.apply(res, p)); | ||
| if (pendingStatus) writeHead.call(res, pendingStatus); |
There was a problem hiding this comment.
I see that @lukeed updated this line in wmr: preactjs/wmr@23bee83
There was a problem hiding this comment.
It's also been updated in Vite's version: https://github.com/vitejs/vite/blob/eef9da13d0028161eacc0ea699988814f29a56e4/packages/vite/src/node/server/middlewares/compression.ts#L80
And the Kikobeats version: https://github.com/Kikobeats/http-compression
|
This version already contains the main fix that Vite copied from wmr (preactjs/wmr#155) when they did their initial commit. It is missing one other fix though that I pointed out above (#148 (comment)). Once that change makes its way in, I think it'd be great to merge and release this! It looks like the version that Vite copied into their codebase almost exactly two years ago has been running there without any fixes necessary in the time since, so this is probably pretty stable. It also looks like this would fix the issue we were having in SvelteKit with the |
|
@benmccann any particular reason you don't want to use Kikobeats/http-compression? Same source code, shipped as npm package, reasy to use. |
|
@Kikobeats thanks for putting this in a module, which seems good way to keep polka small and lean. Then this PR can be closed @developit? |
- now aligns w/ current wmr (and vite) impls
There was a problem hiding this comment.
Thank you @benmccann for the team ping to resurrect this (and multiple others for trying over the lifetime of this PR!)
Long ago, when this PR initially appeared, I had some quirks that I (and rest of wmr team) were trying to chase down. I've had a few iterations of this running in production over the years – but wasn't great at organizing my findings on all of them – and wanted to ensure confidence in this (v important) package before releasing. Great to see that the wmr version (which wasn't this PR) has been proven stable by Vite/WMR/others' usage all this time.
Of course, a massive thank-you to @developit who was brave enough to tackle this problem in the first place & (patiently) entrusted the work over to the Polka household. Thank you 🐐 🦊 🏆
|
Amazing!! Thank you so much @lukeed for getting this across the finish line and making it part of |
This implements gzip + brotli (native) compression as a new
@polka/compressionpackage. It should be ready to go, with tests and docs.