-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
Add deprecation warning to "Referrer" special-casing #4106
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?
Conversation
👌👌 |
re: expressjs#3951 Test plan: `npm test`
af6ad9e
to
9b1c50f
Compare
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.
Based on the linked issue, I think this message isn't happening in the correct life cycle. For example, it show be emitted when either req.get('referer')
or req.get('referrer')
is called when the value returned would be the value from this.headers.referrer
.
The idea is that what is being deprecated is looking at req.headers.referrer
, not just the req.get('referrer')
(which would actually remain and be the way to access the former name.
a9daf54
to
bfb559a
Compare
@dougwilson thanks for the clarification, and sorry for the delay! I just updated the PR. Now, if someone calls:
…the deprecation notice will print. I hope that's what you meant re: lifecycles. As far as I can tell, the only other place this special-casing extends to is |
Also worth noting: tests all pass on this commit, but will need to be updated once the special-casing is removed. |
Hey @rileyjshaw, could you rebase this, please? |
Happy to, @bjohansebas. Does that mean this is getting merged? 😄 |
I'm not sure if this will be included in this release, but it will most likely be in the next ones. |
@@ -14,6 +14,7 @@ | |||
*/ | |||
|
|||
var accepts = require('accepts'); | |||
var deprecate = require('depd')('express'); |
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.
This dependency is no longer in master, please add the dependency back.
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.
Sorry for the delay, I just added it back in.
I am not going to open a blocking review, but if we do decided to land this I think it will be very noisy. Most of the things sending this header in the wild are not things Express application owners can control and so they are highly unlikely to be able to fix. I am not opposed to trying, but I just want to call out that this may be difficult to follow through on removing support for. |
Co-authored-by: Sebastian Beltran <[email protected]>
re: #3951
Test plan:
npm test