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

Better error handling for sendFile #3582

Merged
merged 1 commit into from
Oct 27, 2018
Merged

Conversation

thevoidf
Copy link
Contributor

@thevoidf thevoidf commented Mar 4, 2018

so you don't get confusing errors like: function substring not found or something

Copy link
Contributor

@dougwilson dougwilson left a comment

Choose a reason for hiding this comment

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

Nice! Can you add a test that tests the condition in which you were encountering? That way we don't accidentally regress.

lib/response.js Outdated
if (!path) {
throw new TypeError('path argument is required to res.sendFile');
if (typeof path !== 'string') {
throw new TypeError('path must be string');
Copy link
Contributor

Choose a reason for hiding this comment

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

Also, can you add back res.sendFile to the message? Helps people know what function the issue was to.

@thevoidf
Copy link
Contributor Author

thevoidf commented Mar 4, 2018

if you pass something other than valid string like sendFile({}) you get: TypeError: path.substring is not a function. which is kind of confusing, so instead now you getting: path must be string

@dougwilson
Copy link
Contributor

Makes sense. Just add a test for that so we have it there to not regress from a refactor 👍

@thevoidf
Copy link
Contributor Author

thevoidf commented Mar 4, 2018

@dougwilson all test are passing on my system

@LinusU
Copy link
Member

LinusU commented Mar 4, 2018

The build failures seemed unrelated, I restarted the two failed builds...

@dougwilson dougwilson added this to the 4.17 milestone Mar 12, 2018
@dougwilson dougwilson mentioned this pull request Oct 27, 2018
23 tasks
@dougwilson dougwilson changed the base branch from master to 4.17 October 27, 2018 06:17
@dougwilson dougwilson merged commit 6bcdfef into expressjs:4.17 Oct 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants