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.

Sign up for GitHub

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

Jump to bottom

Send content-length header in method responses #132

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Send content-length header in method responses #132

wants to merge 1 commit into from

Conversation

chfritz
Copy link

@chfritz chfritz commented Jun 28, 2016

  • unlike method calls (client), method responses (server) did not yet
    send content-length headers. This was causing problem with at least
    one client (ros's turtlesim, in the context of rosnodejs: publishing
    to /turtlt1/cmd_vel had no effect on the turtle). This commit fixes
    that.

@chfritz chfritz mentioned this pull request Jun 28, 2016
Merged
@chfritz
Send content-length header in method responses
948db2f 
- unlike method calls (client), method responses (server) did not yet
  send content-length headers. This was causing problem with at least
  one client (ros's turtlesim, in the context of rosnodejs: publishing
  to /turtlt1/cmd_vel had no effect on the turtle). This commit fixes
  that.
@atn18
Copy link

atn18 commented Jun 4, 2019

Hi, there!
Are there any stop factors for merging this PR?

@atn18 atn18 mentioned this pull request Jun 20, 2019
Open
@chfritz chfritz mentioned this pull request Nov 18, 2019
Closed
@rudicus
Copy link

rudicus commented Nov 18, 2019

@agladkov We would love to see this patch pushed. Are there any actual objections?

@atn18
Copy link

atn18 commented Nov 19, 2019

I have no objections.
I have not merge perms :)
Question to @baalexander

@rudicus
Copy link

rudicus commented Nov 19, 2019

@baalexander @patricklodder Any chance you guys can merge this and update the published npm package? We are dependent on a downstream package (https://www.npmjs.com/package/rosnodejs) and I would hate to have to fork both projects for a 1 line change. Thanks.

@cr1979
Copy link

cr1979 commented Mar 5, 2023

I need that change as well!

@chfritz
Copy link
Author

chfritz commented Mar 5, 2023

@cr1979 We've forked the package a while ago and published a new one with the fixes we needed. You can find it here: https://www.npmjs.com/package/xmlrpc-rosnodejs. Just change xmlrpc to xmlrpc-rosnodejs everywhere.

@cr1979
Copy link

cr1979 commented Mar 5, 2023

@cr1979 We've forked the package a while ago and published a new one with the fixes we needed. You can find it here: https://www.npmjs.com/package/xmlrpc-rosnodejs. Just change xmlrpc to xmlrpc-rosnodejs everywhere.

@chfritz perfect thank you!

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

@chris-smith chris-smith chris-smith left review comments

@atn18 atn18 atn18 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@chfritz @atn18 @rudicus @cr1979 @chris-smith