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

feat: add maxReturnSize option #90

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
5 changes: 5 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,11 @@ Provide a max-age in milliseconds for HTTP caching, defaults to 0.
This can also be a string accepted by the
[ms](https://www.npmjs.org/package/ms#readme) module.

##### maxChunkSize

Specify maximum chunk size, defaults to send the entire file size.
This will be used when `acceptRanges` is true.

##### root

Serve files relative to `path`.
Expand Down
8 changes: 8 additions & 0 deletions lib/send.js
Original file line number Diff line number Diff line change
Expand Up @@ -130,12 +130,17 @@ function normalizeOptions (options) {

const maxage = normalizeMaxAge(options.maxAge ?? options.maxage)

const maxChunkSize = options.acceptRanges !== undefined
? Number(options.maxChunkSize)
: null

const root = options.root
? resolve(options.root)
: null

return {
acceptRanges,
maxChunkSize,
ledyba marked this conversation as resolved.
Show resolved Hide resolved
cacheControl,
etag,
dotfiles,
Expand Down Expand Up @@ -546,6 +551,9 @@ function sendFileDirectly (request, path, stat, options) {

// Content-Range
statusCode = 206
if (options.maxChunkSize) {
ranges[0].end = Math.min(ranges[0].end, ranges[0].start + options.maxChunkSize - 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

It doesnt seem right.

Copy link
Author

Choose a reason for hiding this comment

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

There are some unit tests that success, so I think you are pointing out some edge cases that cause bug. If so, could you tell me the condition or something?

}
headers['Content-Range'] = contentRange('bytes', len, ranges[0])

// adjust for requested range
Expand Down
50 changes: 49 additions & 1 deletion test/send.1.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ test('send(file, options)', function (t) {
t.plan(10)

t.test('acceptRanges', function (t) {
t.plan(2)
t.plan(6)

t.test('should support disabling accept-ranges', function (t) {
t.plan(2)
Expand All @@ -37,6 +37,54 @@ test('send(file, options)', function (t) {
.expect(shouldNotHaveHeader('Content-Range', t))
.expect(200, '123456789', err => t.error(err))
})

t.test('should limit chunk size /1', function (t) {
t.plan(4)

request(createServer({ acceptRanges: true, maxChunkSize: 1, root: fixtures }))
.get('/nums.txt')
.set('Range', 'bytes=0-2')
.expect((res) => t.equal(res.headers['accept-ranges'], 'bytes'))
.expect((res) => t.equal(res.headers['content-range'], 'bytes 0-0/9'))
.expect((res) => t.equal(res.headers['content-length'], '1', 'should content-length must be as same as maxChunkSize'))
.expect(206, '1', (err) => t.error(err))
})

t.test('should limit chunk size /2', function (t) {
t.plan(4)

request(createServer({ acceptRanges: true, maxChunkSize: 1, root: fixtures }))
.get('/nums.txt')
.set('Range', 'bytes=1-2')
.expect((res) => t.equal(res.headers['accept-ranges'], 'bytes'))
.expect((res) => t.equal(res.headers['content-range'], 'bytes 1-1/9'))
.expect((res) => t.equal(res.headers['content-length'], '1', 'should content-length must be as same as maxChunkSize'))
.expect(206, '2', (err) => t.error(err))
})

t.test('should limit chunk size /3', function (t) {
t.plan(4)

request(createServer({ acceptRanges: true, maxChunkSize: 1, root: fixtures }))
.get('/nums.txt')
.set('Range', 'bytes=1-3')
.expect((res) => t.equal(res.headers['accept-ranges'], 'bytes'))
.expect((res) => t.equal(res.headers['content-range'], 'bytes 1-1/9'))
.expect((res) => t.equal(res.headers['content-length'], '1', 'should content-length must be as same as maxChunkSize'))
.expect(206, '2', (err) => t.error(err))
})

t.test('should limit chunk size /4', function (t) {
t.plan(4)

request(createServer({ acceptRanges: true, maxChunkSize: 4, root: fixtures }))
.get('/nums.txt')
.set('Range', 'bytes=1-2,3-6')
.expect((res) => t.equal(res.headers['accept-ranges'], 'bytes'))
.expect((res) => t.equal(res.headers['content-range'], 'bytes 1-4/9'))
.expect((res) => t.equal(res.headers['content-length'], '4', 'should content-length must be as same as maxChunkSize'))
.expect(206, '2345', (err) => t.error(err))
})
})

t.test('cacheControl', function (t) {
Expand Down
6 changes: 6 additions & 0 deletions test/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,12 @@ module.exports.shouldNotHaveHeader = function shouldNotHaveHeader (header, t) {
}
}

module.exports.shouldHaveHeader = function shouldHaveHeader (header, t) {
return function (res) {
t.ok((header.toLowerCase() in res.headers), 'should have header ' + header)
}
}

module.exports.createServer = function createServer (opts, fn) {
return http.createServer(async function onRequest (req, res) {
try {
Expand Down
5 changes: 5 additions & 0 deletions types/index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,11 @@ declare namespace send {
*/
maxAge?: string | number | undefined;

/**
* Limit max response size when acceptRanges is true, defaults to the entire file size.
*/
maxChunkSize?: number | undefined;

/**
* Serve files relative to path.
*/
Expand Down
4 changes: 3 additions & 1 deletion types/index.test-d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ const req: any = {}

{
const result = await send(req, '/test.html', {
acceptRanges: true,
maxChunkSize: 10,
immutable: true,
maxAge: 0,
root: __dirname + '/wwwroot'
Expand Down Expand Up @@ -53,4 +55,4 @@ switch (result.type) {
expectType<ErrorSendResult>(result)
expectType<Error>(result.metadata.error)
}
}
}