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

server: send response to client with res.end() #1043

Merged
merged 1 commit into from
Feb 11, 2019

Conversation

peat-psuwit
Copy link
Contributor

Using res.end() to write result instead of res.write() followed by
res.end() will cause node not to use chunked encoding and include
Content-Length header. (See nodejs/node#26005) This small distinction is
important with some client. For example, Windows 10's MDM enrollment
system will not accept chunked response
(https://docs.microsoft.com/en-us/windows/client-management/mdm/on-premise-authentication-device-enrollment).

This might improve compatibility with some other clients, too. However,
there's a small chance that some client may expect the response to be
chunked.

@coveralls
Copy link

coveralls commented Feb 8, 2019

Coverage Status

Coverage increased (+0.003%) to 93.299% when pulling 20390f2 on peat-psuwit:use-res-end-to-write-result into 5b45068 on vpulim:master.

Copy link
Collaborator

@jsdevel jsdevel left a comment

Choose a reason for hiding this comment

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

Can you make this configurable since this is a breaking change?

@peat-psuwit
Copy link
Contributor Author

OK, I'll make this configurable. However, I want your suggestion on option name.

Also, if I add an option, I have to edit the README too, right?

@jsdevel
Copy link
Collaborator

jsdevel commented Feb 8, 2019

Also, if I add an option, I have to edit the README too, right?

yes

OK, I'll make this configurable. However, I want your suggestion on option name.

maybe disableChunkedEncoding? the idea is to keep existing behavior without the presence of the option so existing users aren't effected.

@peat-psuwit
Copy link
Contributor Author

maybe enableChunkedEncoding?

So, server option enableChunkedEncoding with default to true?

@jsdevel
Copy link
Collaborator

jsdevel commented Feb 8, 2019

So, server option enableChunkedEncoding with default to true?

That works

@peat-psuwit peat-psuwit force-pushed the use-res-end-to-write-result branch from d9ed4b5 to 01fb356 Compare February 8, 2019 18:42
Using res.end() to write result instead of res.write() followed by
res.end() will cause node not to use chunked encoding and include
Content-Length header. (See nodejs/node#26005) This small distinction is
important with some client. For example, Windows 10's MDM enrollment
system will not accept chunked response
(https://docs.microsoft.com/en-us/windows/client-management/mdm/on-premise-authentication-device-enrollment).

This might improve compatibility with some other clients, too. However,
there's a small chance that some client may expect the response to be
chunked. So, I put this behind an option which defaults to enabled and
can be disabled if needed.
@peat-psuwit peat-psuwit force-pushed the use-res-end-to-write-result branch from 01fb356 to 20390f2 Compare February 8, 2019 19:02
@jsdevel jsdevel merged commit 6bb6936 into vpulim:master Feb 11, 2019
@jsdevel
Copy link
Collaborator

jsdevel commented Feb 11, 2019

thanks @peat-psuwit !

@peat-psuwit
Copy link
Contributor Author

Thank you for merging my PR, @jsdevel. If you don't mind, could you also take a look at my other PRs, #1022 & #1023, too please?

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.

3 participants