Skip to content

http: add PUT,TRACE methods to support - #2703

Closed
JungMinu wants to merge 1 commit into
nodejs:masterfrom
JungMinu:patch-12
Closed

http: add PUT,TRACE methods to support#2703
JungMinu wants to merge 1 commit into
nodejs:masterfrom
JungMinu:patch-12

Conversation

@JungMinu

Copy link
Copy Markdown
Member

There are two more http methods to support: PUT,TRACE
Add put, trace methods to support

There are two more http methods to support: PUT,TRACE
Add put, trace methods to support
@yorkie

Copy link
Copy Markdown
Contributor

Guess this should be disable

@JungMinu

Copy link
Copy Markdown
MemberAuthor

@yorkie Can I ask you please explain why?

@yorkie

Copy link
Copy Markdown
Contributor

No worries, I found the following line is (just curious which feature do they support?)

self.useChunkedEncodingByDefault = false;

@thefourtheyethefourtheye added the http Issues or PRs related to the http subsystem. label Sep 5, 2015
@JungMinuJungMinu changed the title _http_client: add PUT,TRACE methods to supporthttp: add PUT,TRACE methods to supportSep 5, 2015
@ChALkeR

Copy link
Copy Markdown
Member

Could you provide a testcase that this change fixes?

@JungMinu

Copy link
Copy Markdown
MemberAuthor

@ChALkeR Rather than fixing, this PR provides additional cases for PUT, TRACE methods

@ChALkeR

Copy link
Copy Markdown
Member

@JungMinu Are you sure that they were broken/unsupported without this pull request? If yes, please provide a testcase. If no, I see no point in this PR.

@targos

Copy link
Copy Markdown
Member

@JungMinu This change is not trivial. It needs to be tested. There is already a test for this code path added in the following commit: aef0960.
Looking at it, I think it should fail here because the PUT method is expected to use chunked encoding by default.

What is your reasoning for changing the value of useChunkedEncodingByDefault for PUT and TRACE?

@JungMinu

Copy link
Copy Markdown
MemberAuthor

@targos@ChALkeR Sorry, It was a mistake!

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

httpIssues or PRs related to the http subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@JungMinu@yorkie@ChALkeR@targos@thefourtheye