express
express copied to clipboard
expressjs
location: back override option + tests
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')
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.
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:
- Good idea or not?
- Does this help or change the plan for the removal of the
backmagic string in 5.0? - If good, thoughts on the current name of the option ('location back-referrer'), and if bad, what would be a suggestion?
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.
Removed the non related back tests.
They are in their own Isusue #3292 and RP now #3293
I think there are a couple things that would be good to discuss with the collaborators
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. res.location.back()). This specific PR and workaround for 4.x seems reasonable too, since we can just remove the option in 5.0 when we kill the magic strings.
(for Express 5.0) res.location.back() is not too dissimilar to the browsers window.history.back() so, I guess it would make sense for the people who do both front and back end.
I have not looked at the 5.0 code yet, so I didn't know res.function.function-bolt-on was something the API was going for.
So, was this going to be merged? I got talking to someone today about it and made me think I had already solved this.