Support configured charsets for static content#106
Open
vibhor-aggr wants to merge 2 commits into
Open
Conversation
|
Warning Review limit reached
More reviews will be available in 12 minutes and 58 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Reviewer's GuideAdds optional charset configuration for static file responses, allowing a fixed string or per-file function to append an explicit charset to the Content-Type while preserving existing MIME lookup behavior, with tests and documentation updated accordingly. File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 6 issues, and left some high level feedback:
- By storing the charset-suffixed value in both
obj.typeandobj.mime, any downstream logic that expects a bare MIME type may now receive a fullContent-Type; consider keeping the raw type (e.g. inobj.mime) and only appending the charset at header write time or in a separate field. - The
charsetoption currently passesnamerather than the fullpathnameinto the function; if callers may need the full path or extension, consider clarifying this in the README or passing a more descriptive value such aspathnameor an object containing both.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- By storing the charset-suffixed value in both `obj.type` and `obj.mime`, any downstream logic that expects a bare MIME type may now receive a full `Content-Type`; consider keeping the raw type (e.g. in `obj.mime`) and only appending the charset at header write time or in a separate field.
- The `charset` option currently passes `name` rather than the full `pathname` into the function; if callers may need the full path or extension, consider clarifying this in the README or passing a more descriptive value such as `pathname` or an object containing both.
## Individual Comments
### Comment 1
<location path="index.js" line_range="193-194" />
<code_context>
+ var charset = typeof options.charset === 'function'
+ ? options.charset(name, type)
+ : options.charset
+ obj.type = obj.mime = charset
+ ? type + '; charset=' + charset
+ : type
obj.mtime = stats.mtime
</code_context>
<issue_to_address>
**issue (bug_risk):** Appended `charset` may change existing expectations for `obj.type` / `obj.mime` consumers.
`obj.type`/`obj.mime` used to be a bare MIME type (e.g. `text/html`), but will now sometimes be a full `Content-Type` (e.g. `text/html; charset=utf-8`). Any callers that assume a bare MIME may break or behave incorrectly. If both forms are needed, consider keeping the raw MIME (e.g. `obj.mime`) and adding a separate `obj.contentType` that includes the charset.
</issue_to_address>
### Comment 2
<location path="test/index.js" line_range="153-162" />
<code_context>
})
})
+ it('should serve files with configured charset', function (done) {
+ var app = new Koa()
+ app.use(staticCache(path.join(__dirname, '..'), {
+ charset: 'iso-8859-1',
+ filter(file) {
+ return file === 'README.md'
+ }
+ }))
+ var server = app.listen()
+
+ request(server)
+ .get('/README.md')
+ .expect('Content-Type', 'text/markdown; charset=iso-8859-1')
+ .expect(200, done)
+ })
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test to cover unknown MIME types with a configured charset to ensure the fallback MIME lookup behavior stays intact.
Specifically, please add a test where the served file has an unknown extension (e.g. `foo.bin`) and `options.charset` is set, and assert that the `Content-Type` is `'application/octet-stream; charset=iso-8859-1'` (or equivalent). This will protect the fallback MIME type and charset concatenation behavior from regressions.
Suggested implementation:
```javascript
.expect('Content-Type', 'text/markdown; charset=iso-8859-1')
.expect(200, done)
})
it('should serve files with configured charset for unknown mime types', function (done) {
var app = new Koa()
app.use(staticCache(path.join(__dirname, '..'), {
charset: 'iso-8859-1',
filter(file) {
return file === 'foo.bin'
}
}))
var server = app.listen()
request(server)
.get('/foo.bin')
.expect('Content-Type', 'application/octet-stream; charset=iso-8859-1')
.expect(200, done)
})
```
For this test to pass, ensure there is a `foo.bin` file in the directory being served (based on your existing tests, likely the project root: `path.join(__dirname, '..')`). The file can be empty or contain arbitrary data; it only needs to exist so that the middleware can serve it and trigger the fallback MIME type logic.
</issue_to_address>
### Comment 3
<location path="test/index.js" line_range="183-187" />
<code_context>
+ .expect(200, done)
+ })
+
+ it('should serve files with function configured charset', function (done) {
+ var app = new Koa()
+ app.use(staticCache(path.join(__dirname, '..'), {
+ charset(file, type) {
+ return file === 'README.md' && type === 'text/markdown'
+ ? 'windows-1252'
+ : undefined
+ },
+ filter(file) {
+ return file === 'README.md'
+ }
+ }))
+ var server = app.listen()
+
+ request(server)
+ .get('/README.md')
+ .expect('Content-Type', 'text/markdown; charset=windows-1252')
+ .expect(200, done)
+ })
</code_context>
<issue_to_address>
**suggestion (testing):** Add coverage for the case where the `charset` function returns `undefined` to verify that no charset is appended.
The current test only covers when the charset function returns a string. Please also add a case where `charset(file, type)` returns `undefined` (or another falsy value) and assert that the `Content-Type` header omits `; charset=...`. This documents the fallback behavior and guards against regressions like accidentally sending `; charset=undefined`.
```suggestion
request(server)
.get('/README.md')
.expect('Content-Type', 'text/markdown; charset=windows-1252')
.expect(200, done)
})
it('should not append charset when charset function returns undefined', function (done) {
var app = new Koa()
app.use(staticCache(path.join(__dirname, '..'), {
charset(file, type) {
// explicit undefined to exercise the "no charset" path
return undefined
},
filter(file) {
return file === 'README.md'
}
}))
var server = app.listen()
request(server)
.get('/README.md')
.expect('Content-Type', 'text/markdown')
.expect(200, done)
})
```
</issue_to_address>
### Comment 4
<location path="README.md" line_range="51" />
<code_context>
- `options.cacheControl` (str) - optional cache control header. Overrides `options.maxAge`.
- `options.buffer` (bool) - store the files in memory instead of streaming from the filesystem on each request.
- `options.gzip` (bool) - when request's accept-encoding include gzip, files will compressed by gzip.
+- `options.charset` (str | function) - optional charset appended to `Content-Type`. If a function is supplied, it receives `(file, type)` and can return a charset per file.
- `options.usePrecompiledGzip` (bool) - try use gzip files, loaded from disk, like nginx gzip_static
</code_context>
<issue_to_address>
**issue (typo):** Fix grammar in the gzip option description.
Rephrase to: "when the request's Accept-Encoding includes gzip, files will be compressed using gzip" to fix subject-verb agreement and the missing "be" before "compressed".
```suggestion
- `options.gzip` (bool) - when the request's Accept-Encoding includes gzip, files will be compressed using gzip.
```
</issue_to_address>
### Comment 5
<location path="README.md" line_range="53" />
<code_context>
- `options.buffer` (bool) - store the files in memory instead of streaming from the filesystem on each request.
- `options.gzip` (bool) - when request's accept-encoding include gzip, files will compressed by gzip.
+- `options.charset` (str | function) - optional charset appended to `Content-Type`. If a function is supplied, it receives `(file, type)` and can return a charset per file.
- `options.usePrecompiledGzip` (bool) - try use gzip files, loaded from disk, like nginx gzip_static
- `options.alias` (obj) - object map of aliases. See below.
- `options.prefix` (str) - the url prefix you wish to add, default to `''`.
</code_context>
<issue_to_address>
**issue (typo):** Add missing "to" in the usePrecompiledGzip description.
Change "try use gzip files" to "try to use gzip files" for correct grammar.
```suggestion
- `options.usePrecompiledGzip` (bool) - try to use gzip files, loaded from disk, like nginx gzip_static
```
</issue_to_address>
### Comment 6
<location path="README.md" line_range="55" />
<code_context>
+- `options.charset` (str | function) - optional charset appended to `Content-Type`. If a function is supplied, it receives `(file, type)` and can return a charset per file.
- `options.usePrecompiledGzip` (bool) - try use gzip files, loaded from disk, like nginx gzip_static
- `options.alias` (obj) - object map of aliases. See below.
- `options.prefix` (str) - the url prefix you wish to add, default to `''`.
</code_context>
<issue_to_address>
**suggestion (typo):** Improve capitalization and verb agreement in the prefix description.
Consider capitalizing "URL" and fixing the verb agreement, for example: "the URL prefix you wish to add, defaults to `''`."
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+153
to
+162
| it('should serve files with configured charset', function (done) { | ||
| var app = new Koa() | ||
| app.use(staticCache(path.join(__dirname, '..'), { | ||
| charset: 'iso-8859-1', | ||
| filter(file) { | ||
| return file === 'README.md' | ||
| } | ||
| })) | ||
| var server = app.listen() | ||
|
|
There was a problem hiding this comment.
suggestion (testing): Add a test to cover unknown MIME types with a configured charset to ensure the fallback MIME lookup behavior stays intact.
Specifically, please add a test where the served file has an unknown extension (e.g. foo.bin) and options.charset is set, and assert that the Content-Type is 'application/octet-stream; charset=iso-8859-1' (or equivalent). This will protect the fallback MIME type and charset concatenation behavior from regressions.
Suggested implementation:
.expect('Content-Type', 'text/markdown; charset=iso-8859-1')
.expect(200, done)
})
it('should serve files with configured charset for unknown mime types', function (done) {
var app = new Koa()
app.use(staticCache(path.join(__dirname, '..'), {
charset: 'iso-8859-1',
filter(file) {
return file === 'foo.bin'
}
}))
var server = app.listen()
request(server)
.get('/foo.bin')
.expect('Content-Type', 'application/octet-stream; charset=iso-8859-1')
.expect(200, done)
})For this test to pass, ensure there is a foo.bin file in the directory being served (based on your existing tests, likely the project root: path.join(__dirname, '..')). The file can be empty or contain arbitrary data; it only needs to exist so that the middleware can serve it and trigger the fallback MIME type logic.
Comment on lines
+183
to
+187
| request(server) | ||
| .get('/README.md') | ||
| .expect('Content-Type', 'text/markdown; charset=windows-1252') | ||
| .expect(200, done) | ||
| }) |
There was a problem hiding this comment.
suggestion (testing): Add coverage for the case where the charset function returns undefined to verify that no charset is appended.
The current test only covers when the charset function returns a string. Please also add a case where charset(file, type) returns undefined (or another falsy value) and assert that the Content-Type header omits ; charset=.... This documents the fallback behavior and guards against regressions like accidentally sending ; charset=undefined.
Suggested change
| request(server) | |
| .get('/README.md') | |
| .expect('Content-Type', 'text/markdown; charset=windows-1252') | |
| .expect(200, done) | |
| }) | |
| request(server) | |
| .get('/README.md') | |
| .expect('Content-Type', 'text/markdown; charset=windows-1252') | |
| .expect(200, done) | |
| }) | |
| it('should not append charset when charset function returns undefined', function (done) { | |
| var app = new Koa() | |
| app.use(staticCache(path.join(__dirname, '..'), { | |
| charset(file, type) { | |
| // explicit undefined to exercise the "no charset" path | |
| return undefined | |
| }, | |
| filter(file) { | |
| return file === 'README.md' | |
| } | |
| })) | |
| var server = app.listen() | |
| request(server) | |
| .get('/README.md') | |
| .expect('Content-Type', 'text/markdown') | |
| .expect(200, done) | |
| }) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #83.
This adds an optional
charsetsetting for static file responses so applications that know their file encoding can return an explicitContent-Typecharset instead of relying on Koa's default behavior.What changed:
options.charsetsupport as either a string or a(file, type)functionI kept this configurable rather than attempting automatic charset detection because the middleware does not have reliable encoding metadata for arbitrary static files.
Verification:
npm testgit diff --checkSummary by Sourcery
Add optional charset configuration for static file responses and document and test the new behavior.
New Features:
Documentation:
Tests: