-
-
Notifications
You must be signed in to change notification settings - Fork 16.8k
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
location: back override option + tests #3291
base: master
Are you sure you want to change the base?
Conversation
Adding the missing tests is an obvious thing to immediately merge, so I wish adding tests for the existing stuff was a separate PR from the override option so I could at least merge them while waiting for feedback on the added option. I can split them up myself, but it would cause a merge conflict on this PR if I did that. |
I will split once I'm home |
Actually, it looks like I can commit to your fork's branch from the new GitHub functionality, so I can split them for you if you don't get around to it :) Anyway, on the topic of the new option, I think there are a couple things that would be good to discuss with the collaborators:
I know some people may know my thoughts on the magic back string in general, but to reiterate: I definitely don't like it because there is no way to distinguish between wanting the referring behavior vs generating a location of a literal "back" (I think as @WORMSS experienced). As for adding an option for controlling the behavior in general, I'm neutral on it right now, but certainly don't see the harm. This option is currently implemented in the PR such that it will not inherit to sub apps, which is what I would expect for this setting (to be app-isolated) since you can't be sure what a sub app you're including is expecting to do. |
A split of expressjs#3291 of just the tests. Fixes expressjs#3292
Do we have an open discussion around the use of magic strings already? In general, I'd love to kill them all from the codebase and just use functions instead (e.g. |
(for Express 5.0) I have not looked at the 5.0 code yet, so I didn't know |
Added tests for res.location('back') Made option to turn off built 'back' referrer functionality when not working the way it was expected. fixes: expressjs#3290
779f541
to
28d27b2
Compare
So, was this going to be merged? I got talking to someone today about it and made me think I had already solved this. |
eb10dba
to
340be0f
Compare
Make option for setting back as location to override built in behavior. To redirect to page "back", default behavior is to use referrer or navigate to root of domain and not the intended relative path. This can be compensated for with "./back" but when dealing with third party responses, you would need to test for "back" on every call to replace.
Added tests for location('back')