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

fix(core): apply existing headers to sse responses #8729

Merged
merged 1 commit into from
Dec 6, 2021

Conversation

wSedlacek
Copy link
Contributor

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

What is the current behavior?

When sending an sse response with fastify no headers applied to the fastify reply would be applied to the headers of the sse response

Issue Number: #8717

What is the new behavior?

Any headers from the express response or fastify reply will be read and then applied to the sse response.
This has been tested with express and fastify with app.enableCors()

Screen Shot 2021-11-29 at 20 14 22

Does this PR introduce a breaking change?

  • Yes
  • No

@coveralls
Copy link

Pull Request Test Coverage Report for Build 0b4550b8-00c4-4985-80a5-b2f9b2067082

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 94.08%

Totals Coverage Status
Change from base Build e6351c6b-4e81-4159-bd7d-395c4d0911fa: 0.0%
Covered Lines: 5658
Relevant Lines: 6014

💛 - Coveralls

Copy link
Member

@jmcdo29 jmcdo29 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for adding some typings and tests around this

@wSedlacek
Copy link
Contributor Author

@jmcdo29 What's the typical timeline on bugs like this?

@jmcdo29
Copy link
Member

jmcdo29 commented Dec 4, 2021

Depends on how busy the week is for Kamil. Sometimes it's the same day, sometimes it's a week or two. I'll try to point this out to him next week if he doesn't get to it over the weekend

@kamilmysliwiec kamilmysliwiec merged commit ef12ab3 into nestjs:master Dec 6, 2021
@kamilmysliwiec
Copy link
Member

LGTM

@wSedlacek wSedlacek deleted the fix/sse-headers branch December 6, 2021 15:50
@maxsimych
Copy link

@kamilmysliwiec Hello, sorry for disturbing, This bug was critical for some projects, including ours, do you have ability to tag this version, to allow us to upgrade dependency from npm. Thank you anyway!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants