elysiajs / elysia

Ergonomic Framework for Humans
https://elysiajs.com
MIT License
10.25k stars 219 forks source link

BUG: Headers being duplicated after v0.6.24 #178

Closed arthurfiorette closed 11 months ago

arthurfiorette commented 1 year ago

After I upgraded elysiajs to v0.6.24. Most of https://github.com/elysiajs/elysia-html tests are failing due to headers being duplicated.

image

You can reproduce it by cloning the repo and running the tests with v0.6.24 and v0.6.23.

Current elysiajs/html has some bugs in the headers workflow but that's not the case here as with .23 tests are passing. I did not have time do debug this issue within the elysia's codebase, but may just be that the way derive is used with headers changed from .23 to .24 and it needs to be updated inside the html plugin.

P.S: Is there a reason to not have CI pipelines? This could've been caught automatically in elysiajs/html codebase if automatic tests were correctly set up. (#179)

bogeychan commented 1 year ago

This PR causes the new bug https://github.com/elysiajs/elysia/pull/167

The following lines of code in mapEarlyResponse breaks it: https://github.com/elysiajs/elysia/blob/4d740866da5dac28e53ec0926717db28e1ee2e77/src/handler.ts#L73-L74

The Content-Type Header exists in both inherits and response.headers.

See

We need to somehow merge the headers or define whether it is allowed to use new Response(response, set).

idk if it can be merged at all, in case someone needs duplicate headers (name and value?).. any ideas @SaltyAom

arthurfiorette commented 1 year ago

Why is merge the default behavior? There's little to none headers where this would be useful. Server, content length, content type, authorization and many others in tests are also being duplicated.

The current Elysia html implementation is faulty, however it is also breaking when multiple libraries interact with each other and in tests. Headers by default should overwrite unless .append method is manually called somewhere.

bogeychan commented 1 year ago

Merge

Using merge as the default to be some fault tolerant. the headers in the response itself would have a higher priority, like (pseudo code):

for (const key in inherits) {
  if (response.headers.has(key)) continue;
  response.headers.append(key, inherits[key]);
}

Define

but it's also valid to define that when someone returns a Response object they must take care of setting the headers:

return new Response(response, set)

and therefore remove these lines of code from mapEarlyResponse:

https://github.com/elysiajs/elysia/blob/4d740866da5dac28e53ec0926717db28e1ee2e77/src/handler.ts#L73-L74

SaltyAom commented 11 months ago

Should be fixed in 0fc2f0a, and seems not to happen in the latest version.

Feels free to reopen the issue if still persists.