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

Should be implemented as a .mapResponse instead of .onAfterHandle #29

Closed
M-Gonzalo opened this issue Jun 22, 2024 · 14 comments
Closed

Should be implemented as a .mapResponse instead of .onAfterHandle #29

M-Gonzalo opened this issue Jun 22, 2024 · 14 comments

Comments

@M-Gonzalo
Copy link
Contributor

M-Gonzalo commented Jun 22, 2024

This plugin interferes with other middlewares that might return a response early on the lifecycle.

I'm writing a caching plugin, and it needs to access the status of the request to know if it can save the response. But elysia-compress sets all headers to undefined. So approximately half of the requests are never cached. This ratio might have something to do with the order Elysia calls the plugins.

The docs state that it's recommended to compress the data after it's handled by any handler function (including an afterHandle) https://elysiajs.com/life-cycle/map-response.html

Every afterHandle function that returns something halts the processing of the request and the returned value is sent to the client.

PS: I also implemented a caching system for your plugin so it doesn't have to compress every request all the time. Along with my normal cache plugin, it makes for micro second response times. I'll submit a PR after this issue is fixed

@M-Gonzalo M-Gonzalo changed the title Should be implemented as a a .mapResponse instead of .onAfterHandle Should be implemented as a .mapResponse instead of .onAfterHandle Jun 23, 2024
@M-Gonzalo
Copy link
Contributor Author

OK I'm here to correct myself. The docs say that:

Unlike beforeHandle, after a value is returned from afterHandle, the iteration of afterHandle will NOT be skipped.

So apparently, more than one .onAfterHandle plugins can coexist. We just have to find a way of maybe not overwriting some values other plugins might need.

Why is that you set all those headers to undefined?

@vermaysha
Copy link
Owner

at this point for all headers will be sent via Response, so context.set.headers will be sent as undefined.

but I think this is a bad way, so I tried to reverse it, so that all headers will only be sent through context.set.headers in this PR #32

@M-Gonzalo
Copy link
Contributor Author

at this point for all headers will be sent via Response, so context.set.headers will be sent as undefined.

but I think this is a bad way, so I tried to reverse it, so that all headers will only be sent through context.set.headers in this PR #32

That should solve my problem, yes, thanks. How did you manage to make .mapResponse work though? I tried to write a compressor myself but couldn't get past the minimal example in the docs. It sends an empty response. I filed a bug report here in the Discord server if you want to look at it

@vermaysha
Copy link
Owner

at this point for all headers will be sent via Response, so context.set.headers will be sent as undefined.
but I think this is a bad way, so I tried to reverse it, so that all headers will only be sent through context.set.headers in this PR #32

That should solve my problem, yes, thanks. How did you manage to make .mapResponse work though? I tried to write a compressor myself but couldn't get past the minimal example in the docs. It sends an empty response. I filed a bug report here in the Discord server if you want to look at it

If the latest patch I made can solve the problem that occurred, then I will immediately publish it after doing some testing once again.

@M-Gonzalo
Copy link
Contributor Author

M-Gonzalo commented Jun 24, 2024

ok, I renamed main.js and introduced main.ts. I'm getting the same errors I got for my barebones compressor, what I documented on Discord

Decompression error: BrotliDecompressionError
BrotliDecompressionError: BrotliDecompressionError fetching "http://0.0.0.0:1500/api/locations". For more information, pass `verbose: true` in the second argument to fetch()
 path: "http://0.0.0.0:1500/api/locations"

Bun v1.1.17-canary.1+314666d8a (Linux x64)

Same if I try gzip or deflate, it only changes the name of the error.

I think there's something broken in Elysia itself. Not in your code

@vermaysha
Copy link
Owner

Closed due to fix #32

@M-Gonzalo
Copy link
Contributor Author

M-Gonzalo commented Jun 24, 2024

Closed due to fix #32

It's not working though... @vermaysha

@vermaysha vermaysha reopened this Jun 24, 2024
@vermaysha
Copy link
Owner

vermaysha commented Jun 24, 2024

can you provide reproduce step ? or write unit test ?

@M-Gonzalo
Copy link
Contributor Author

can you provide reproduce step ? or write unit test ?

I left it documented on Discord https://discord.com/channels/1044804142461362206/1254602130870767677

@M-Gonzalo
Copy link
Contributor Author

can you provide reproduce step ? or write unit test ?

I left it documented on Discord discord.com/channels/1044804142461362206/1254602130870767677

const { PORT } = Bun.env
import { Elysia } from 'elysia'

import { Logestic } from 'logestic'
import { cors } from '@elysiajs/cors'

import apiRoute from '@/routes/apiRoute'
import staticRoute from '@/routes/staticRoute'

import errorLogger from '@/plugins/errorLogger'

// import zlib from 'zlib'
// import { CompressionOptions, compression } from 'elysia-compress'
// const compressionOptions = {
//     threshold: 256,
//     // encodings: ['gzip', 'deflate'],  // brotli can be quite resource-hungry
//     brotliOptions: {
//         params: {
//             [zlib.constants.BROTLI_PARAM_QUALITY]: 6,
//             [zlib.constants.BROTLI_PARAM_MODE]: zlib.constants.BROTLI_MODE_TEXT,
//         }
//     }
// } as CompressionOptions

export default (startTime: number) => new Elysia()
    // Middlewares:
    .use(cors())
    .use(errorLogger)
    .use(Logestic.preset('fancy'))
    // .use(compression(compressionOptions))
    // Routes:
    .use(apiRoute)
    .use(staticRoute)
    // Start the server:
    .listen(PORT || 3000, ({ hostname, port }) => {
        const startupTime = (performance.now() - startTime).toFixed(2)
        console.log(`🦊 API running at ${hostname}:${port} (${startupTime}ms)`)
    })

@M-Gonzalo
Copy link
Contributor Author

Starter:

import loadRedis from '@/loaders/loadRedis'
import loadMongoDB from '@/loaders/loadMongoDB'
import startElysia from '@/loaders/loadElysiaJS'

export default Promise.resolve(performance.now())
  .then(loadRedis).then(loadMongoDB).then(startElysia)

@M-Gonzalo
Copy link
Contributor Author

@vermaysha I think this got fixed with the latest update to Elysia (1.0.26). At least for me it's working. No need to change anything on your end

@vermaysha
Copy link
Owner

@vermaysha I think this got fixed with the latest update to Elysia (1.0.26). At least for me it's working. No need to change anything on your end

ahh i see, im confused with that

@vermaysha
Copy link
Owner

case closed

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

No branches or pull requests

2 participants