-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
remove compression
middleware from adapter-node output server
#5506
Conversation
🦋 Changeset detectedLatest commit: 90f1764 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I much prefer the original PR #5491. If we want to avoid the option, maybe we could merge this PR and then revert it after compression
is fixed?
Adding options for this sort of thing is just (sorry) bad API design. It's out of place, and having it be a boolean value assumes that we know best what options to set for |
Sure, but it doesn't have to be a boolean. We can pass through the compression options by making it a build time option instead of runtime option |
I'm more on the idea of letting end-user play with middlewares each one considers necessary for each case (instrumentation, logging, compression, rate limit, tenant handling) +1 to letting us configure middleware layer on build time @benmccann |
We decided to go ahead with this for now, just so we have a working version. Whether and how we reinstate it is still up for debate |
Sad panda on this one. Anyone have a viable alternative to Also, does anyone know why this only started happening with the Vite 3 release? |
It's not related to Vite 3, but because SvelteKit started streaming recently and compression's streaming implementation is broken. We're waiting for the fix: expressjs/compression#183 |
Is there some alternative which could be applied in mean time to prevent the node hanging issue ? Most of us who use Sveltekit on production environment are currently completely blocked with the streaming & compression change. We have to keep Kit pinned at |
Compression has been removed, so if you update to the latest it will work. You can do compression in your load balancer or a hook if you need. We can consider adding compression back after it's been fixed |
If someone can figure out a way to implement that hook, I'd be interested. It's pretty crazy how many of the frameworks use the I host on Google Cloud Run which doesn't have an option to add compression so my users are sort of stuck with large payloads till there's a workaround. |
@coryvirok an alternative could be to wrap the sveltekit handler (generated by the adapter) with a custom server implementation and in this server u need to enable the comoression middleware. below an example, anyway keep in mind that there is a bug on the compression lib /* global ENV_PREFIX */
import { handler } from './handler.js';
import { env } from './env.js';
import compression from 'compression';
import polka from 'polka';
export const path = env('SOCKET_PATH', false);
export const host = env('HOST', '0.0.0.0');
export const port = env('PORT', !path && '3000');
export const compression_enabled = env('COMPRESSION_ENABLED', 'true') === 'true';
export const compression_threshold = parseInt(env('COMPRESSION_THRESHOLD', '0'), 10);
if (isNaN(compression_threshold) || compression_threshold < 0) {
throw Error(`${ENV_PREFIX}COMPRESSION_THRESHOLD should be a positve number`);
}
const compression_middleware = compression_enabled
? compression({ threshold: compression_threshold })
: undefined;
const middlewares = [compression_middleware, handler].filter(Boolean);
// https://github.com/lukeed/polka/issues/173
// @ts-ignore - nothing we can do about so just ignore it
const server = polka().use(...middlewares);
server.listen({ path, host, port }, () => {
console.log(`Listening on ${path ? path : host + ':' + port}`);
});
export { server }; |
This is an alternative to #5491. Adding a new option that adds or disables the middleware feels like a bad precedent, and we've often wondered if it really belongs here (rather than at the load balancer level, for example).
Since we clearly document how to use the exported handler with a custom server, I think it's safe to err on the side of simplicity and just get rid of it
Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
Tests
pnpm test
and lint the project withpnpm lint
andpnpm check
Changesets
pnpm changeset
and following the prompts. All changesets should bepatch
until SvelteKit 1.0