Skip to content

Remove res.vary() (no arguments) signature #2943

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Remove res.vary() (no arguments) signature #2943

wants to merge 1 commit into from

Conversation

tunniclm
Copy link
Contributor

Changed res.vary() to throw TypeError if no field argument is passed or if the value of field argument is an empty array. (Similar to how res.sendFile() deals with a missing path argument).

@dougwilson
Copy link
Contributor

The vary module already does this check and throw, no?

@tunniclm
Copy link
Contributor Author

It does throw if field is missing, but not if it is an empty array (I think it does nothing in that case). Maybe that is ok?

@tunniclm
Copy link
Contributor Author

Happy to remove the check/throw or change it if it is still good to test if field is an empty array. I guess that check was only really to catch a deprecated use case, so I imagine the check can just be removed entirely.

@dougwilson
Copy link
Contributor

Yea, the idea behind the change is to remove any logic from Express itself, and just have it pass-through to the dependency.

@tunniclm tunniclm force-pushed the remove_deprecated_res_vary branch from 2dd248a to f5bde96 Compare March 14, 2016 16:07
@tunniclm
Copy link
Contributor Author

OK, updates. I've removed the test that passes an empty array and kept the no args test. I think that's ok, but I'm happy to either remove the no args or add a test for empty array that checks it has no effect, if that fits better.

@dougwilson
Copy link
Contributor

add a test for empty array that checks it has no effect, if that fits better.

Yea, let's keep the test, but update the expected outcome to what it is with the change.

@tunniclm tunniclm force-pushed the remove_deprecated_res_vary branch from f5bde96 to 447d813 Compare March 14, 2016 16:23
@tunniclm
Copy link
Contributor Author

OK, done.

@tunniclm tunniclm force-pushed the remove_deprecated_res_vary branch from 447d813 to bd2fc86 Compare March 14, 2016 16:24
@tunniclm
Copy link
Contributor Author

shouldNotHaveHeader() is no longer used, so I removed that as well.

@tunniclm tunniclm force-pushed the remove_deprecated_res_vary branch from bd2fc86 to 0f34edb Compare April 5, 2016 16:30
@hacksparrow
Copy link
Member

@tunniclm coverage has decreased by a fraction, otherwise looks good to me.

@tunniclm
Copy link
Contributor Author

tunniclm commented May 4, 2016

As mentioned in the other similar pull request (see #2939 (comment)), the slight reduction in coverage is due to the reduced number of lines in the changed file (response.js).

@dougwilson dougwilson added the 5.x label Jun 20, 2016
@dougwilson dougwilson self-assigned this Jun 20, 2016
@dougwilson dougwilson added the pr label Jun 20, 2016
dougwilson
dougwilson reviewed Jun 20, 2016
View reviewed changes
test/res.vary.js
@@ -5,7 +5,7 @@ var request = require('supertest');

describe('res.vary()', function(){
describe('with no arguments', function(){
it('should not set Vary', function (done) {
it('should error missing field', function (done) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add this as an additional test rather than altering the existing test? I think both the former test and the test you changed it to have merit and would like to have both.

@dougwilson
Copy link
Contributor

Hi @tunniclm, can you address the comment regarding the test and possibly rebase this pull request? Then I think everything is resolved to merge after that.

@dougwilson dougwilson mentioned this pull request Jun 20, 2016
Merged
39 tasks
@dougwilson dougwilson added this to the 5.0 milestone Jun 20, 2016
@tunniclm tunniclm force-pushed the remove_deprecated_res_vary branch from 0f34edb to 56e6bba Compare June 20, 2016 09:08
@tunniclm
Copy link
Contributor Author

Updated.

dougwilson pushed a commit that referenced this pull request Jan 28, 2017
@tunniclm @dougwilson
Remove res.vary() (no arguments) signature
a856456 
closes #2943
@dougwilson
Copy link
Contributor

Merged into the 5.0 branch.

@dougwilson dougwilson closed this Jan 28, 2017
@snyk-bot snyk-bot mentioned this pull request Jun 29, 2022
Open
@Vanessa-H Vanessa-H mentioned this pull request Jul 5, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@dougwilson dougwilson

Labels
5.x pr
Projects
None yet
Milestone
5.0
Development

Successfully merging this pull request may close these issues.
3 participants
@tunniclm @dougwilson @hacksparrow