Skip to content

Send Accept-Ranges: bytes with every response #536

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

Merged
merged 4 commits into from
Dec 30, 2019
Merged

Send Accept-Ranges: bytes with every response #536

merged 4 commits into from
Dec 30, 2019

Conversation

boramalper
Copy link
Contributor

Please ensure that your pull request fulfills these requirements:

  • The pull request is being made against the master branch
  • Tests for the changes have been added (for bug fixes / features)

What is the purpose of this pull request? (bug fix, enhancement, new feature,...)

Enhancement

What changes did you make?

Now Accept-Ranges: bytes header is sent with every response by default.

Provide some example code that this change will affect, if applicable:

N/A

Is there anything you'd like reviewers to focus on?

I think sending Accept-Ranges: bytes with every response is pretty benign, but surely the behaviour of some programs might be affected with this header (i.e. they might try to stream instead of downloading the whole file). If this is a concern, a new command-line flag can be added.

Please provide testing instructions, if applicable:

N/A

@thornjad
Copy link
Member

What does this header mean for the client? What does it do?

@boramalper
Copy link
Contributor Author

Copy link
Member

@thornjad thornjad left a comment

Choose a reason for hiding this comment

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

Since this is to be sent with every request, it might be better to put it into the main lib/http-server.js rather than the CLI driver in bin/. Otherwise, pretty nice! It should be pretty benign for most uses, but I'm going to mark it as a major change just in case.

@thornjad thornjad added feature:http major version Major, potentially breaking, change labels Dec 13, 2019
@thornjad
Copy link
Member

Would this fix #257?

@boramalper
Copy link
Contributor Author

Sorry I no longer have the repository on my computer (removed it during a migration). It's a trivial change, so please feel free to go ahead and implement it yourself (without giving "credit"). =)

@thornjad
Copy link
Member

Okay, sure I'll take over this PR!

@thornjad thornjad added this to the v0.13.0 milestone Dec 26, 2019
@thornjad
Copy link
Member

Presuming the tests pass, I'll approve this PR; thanks for your addition! I made a couple changes to your fork directly so we'll keep history and credit steady.

Copy link
Member

@thornjad thornjad left a comment

Choose a reason for hiding this comment

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

Changes staged for v0.13.0

@thornjad thornjad changed the base branch from master to rc-v0.13.0 December 30, 2019 15:01
@thornjad thornjad merged commit e2569f5 into http-party:rc-v0.13.0 Dec 30, 2019
@boramalper
Copy link
Contributor Author

Thanks! <3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major version Major, potentially breaking, change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants