-
-
Notifications
You must be signed in to change notification settings - Fork 54
feat: add contentTypeNegotiation option #131
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -11,6 +11,7 @@ | |||||||||||||||||||||||||
| * @private | ||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| const Negotiator = require('negotiator') | ||||||||||||||||||||||||||
| var debug = require('debug')('finalhandler') | ||||||||||||||||||||||||||
| var encodeUrl = require('encodeurl') | ||||||||||||||||||||||||||
| var escapeHtml = require('escape-html') | ||||||||||||||||||||||||||
|
|
@@ -25,28 +26,35 @@ var statuses = require('statuses') | |||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| var isFinished = onFinished.isFinished | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| const AVAILABLE_MEDIA_TYPES = ['text/plain', 'text/html'] | ||||||||||||||||||||||||||
| const HTML_CONTENT_TYPE = 'text/html; charset=utf-8' | ||||||||||||||||||||||||||
| const TEXT_CONTENT_TYPE = 'text/plain; charset=utf-8' | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||
| * Create a minimal HTML document. | ||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||
| * @param {string} message | ||||||||||||||||||||||||||
| * @private | ||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||
|
Comment on lines
33
to
38
|
||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| function createHtmlDocument (message) { | ||||||||||||||||||||||||||
| var body = escapeHtml(message) | ||||||||||||||||||||||||||
| function createHtmlBody (message) { | ||||||||||||||||||||||||||
| const msg = escapeHtml(message) | ||||||||||||||||||||||||||
| .replaceAll('\n', '<br>') | ||||||||||||||||||||||||||
| .replaceAll(' ', ' ') | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| return '<!DOCTYPE html>\n' + | ||||||||||||||||||||||||||
| '<html lang="en">\n' + | ||||||||||||||||||||||||||
| '<head>\n' + | ||||||||||||||||||||||||||
| '<meta charset="utf-8">\n' + | ||||||||||||||||||||||||||
| '<title>Error</title>\n' + | ||||||||||||||||||||||||||
| '</head>\n' + | ||||||||||||||||||||||||||
| '<body>\n' + | ||||||||||||||||||||||||||
| '<pre>' + body + '</pre>\n' + | ||||||||||||||||||||||||||
| '</body>\n' + | ||||||||||||||||||||||||||
| '</html>\n' | ||||||||||||||||||||||||||
| const html = `<!DOCTYPE html> | ||||||||||||||||||||||||||
| <html lang="en"> | ||||||||||||||||||||||||||
| <head> | ||||||||||||||||||||||||||
| <meta charset="utf-8"> | ||||||||||||||||||||||||||
| <title>Error</title> | ||||||||||||||||||||||||||
| </head> | ||||||||||||||||||||||||||
| <body> | ||||||||||||||||||||||||||
| <pre>${msg}</pre> | ||||||||||||||||||||||||||
| </body> | ||||||||||||||||||||||||||
| </html> | ||||||||||||||||||||||||||
| ` | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| return Buffer.from(html, 'utf8') | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||
|
|
@@ -75,6 +83,15 @@ function finalhandler (req, res, options) { | |||||||||||||||||||||||||
| // get error callback | ||||||||||||||||||||||||||
| var onerror = opts.onerror | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| // fallback response content type negotiation enabled | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
| // fallback response content type negotiation enabled | |
| // response content type negotiation enabled |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment "If text/plain fallback is enabled" is misleading. The option is named contentTypeNegotiation, not "text/plain fallback", and it enables negotiation between both text/plain and text/html. Consider updating the comment to "If content type negotiation is enabled" for clarity.
| // If text/plain fallback is enabled, negotiate content type | |
| // If content type negotiation is enabled, negotiate content type |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment "default to plain text" is misleading in this context. This case is reached when preferredType is 'text/plain' OR when defaultContentType is 'text/plain' (and no preference was determined). The comment should be removed or clarified to "text/plain response" to avoid confusion.
| // default to plain text | |
| // text/plain response |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The switch statement lacks a default case, which could result in body and contentType being undefined if an unexpected value is used. While the validation at line 91-93 should prevent this, adding a default case would make the code more defensive and prevent potential runtime errors if the validation logic changes in the future.
| break | |
| break | |
| default: | |
| // Defensive fallback: use the configured defaultContentType | |
| if (defaultContentType === 'text/plain') { | |
| body = Buffer.from(msg, 'utf8') | |
| contentType = TEXT_CONTENT_TYPE | |
| } else { | |
| body = createHtmlBody(msg) | |
| contentType = HTML_CONTENT_TYPE | |
| } | |
| break |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation for
contentTypeNegotiationshould clarify the behavior when no Accept header is present. Based on the test at line 433, when content negotiation is enabled and there's no Accept header, the response defaults to 'text/plain' (the first item in AVAILABLE_MEDIA_TYPES). This should be explicitly documented to avoid confusion.