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

Fixed win10 cannot handle EventSource Response #352

Closed
wants to merge 2 commits into from
Closed

Fixed win10 cannot handle EventSource Response #352

wants to merge 2 commits into from

Conversation

max918
Copy link

@max918 max918 commented Apr 12, 2019

This PR contains a:

  • bugfix
  • new feature
  • code refactor
  • test update
  • typo fix
  • metadata update

Motivation / Use-Case

on Window 10, browsers would not process (or cannot handle) the EventSource's HTTP response if the response is without 'Content-Length'. It cause the client.js cannot be notified to update frontend changes.

But hot-middleware is using the streaming data approach to achieve eventsource. So, adding the Response Header 'Content-Length: -1' can solve the problem.

The fix is already tested on Chrome browser of window 10 (64 bit) and Mac OS.

#101

Breaking Changes

Additional Info

Issue: on Window 10, browsers would not process (or cannot handle) the EventSource's HTTP response if the response is without 'Content-Length'. But hot-middleware is using the streaming data approach to achieve eventsource. So, adding the Response Header 'Content-Length: -1' can solve the problem.
Fixed win10 cannot handle EventSource Response
@jsf-clabot
Copy link

jsf-clabot commented Apr 12, 2019

CLA assistant check
All committers have signed the CLA.

@alexander-akait
Copy link
Member

Can you create minimum reproducible test repo, adding this header is breaking change and break logic for other types

Copy link

@thfai2000 thfai2000 left a comment

Choose a reason for hiding this comment

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

@max918
It may be a better way for developer to pass additional headers instead of strictly changing the 'Content-Length'.

// add below code at Line 92 of middleware.js
Object.assign(headers, opts.additionalHeaders || {});

So that you can pass 'Content-Length' in the middleware configuration for your needs

{
  "additionalHeaders": {
    "Content-Length": -1
  }
}

@max918 max918 closed this by deleting the head repository Oct 28, 2023
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

Successfully merging this pull request may close these issues.

None yet

4 participants