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

location: back override option + tests #3291

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

WORMSS
Copy link
Contributor

@WORMSS WORMSS commented Apr 27, 2017

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')

@dougwilson
Copy link
Contributor

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.

@WORMSS
Copy link
Contributor Author

WORMSS commented Apr 27, 2017

I will split once I'm home

@dougwilson
Copy link
Contributor

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:

  1. Good idea or not?
  2. Does this help or change the plan for the removal of the back magic string in 5.0?
  3. 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.

WORMSS added a commit to WORMSS/express that referenced this pull request Apr 27, 2017
A split of expressjs#3291 of just the tests.

Fixes expressjs#3292
@WORMSS WORMSS mentioned this pull request Apr 27, 2017
@WORMSS
Copy link
Contributor Author

WORMSS commented Apr 27, 2017

Removed the non related back tests.

They are in their own Isusue #3292 and RP now #3293

@blakeembrey
Copy link
Member

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.

@WORMSS
Copy link
Contributor Author

WORMSS commented Apr 28, 2017

(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.

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
@WORMSS
Copy link
Contributor Author

WORMSS commented Sep 13, 2018

So, was this going to be merged? I got talking to someone today about it and made me think I had already solved this.

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