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

Handle paths in redirection #353

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

christianwerz
Copy link

Inspired by #16

This allows you to do http://localhost:2000/cheatsheets/XYZ.html and have it redirect to http://localhost:68192/XYZ.html.

add redirect with 307
add path to target
@dbraley
Copy link

dbraley commented Feb 20, 2020

Ha, I was just about to submit a PR that did exactly this (plus offered the ability to have servers configured to forward by proxy rather than redirect). Wish I'd seen this PR first, would have saved myself some time! @typicode This goes to addressing my concerns in Issue #350.

@dbraley
Copy link

dbraley commented Feb 20, 2020

Something minor review-like things I noticed when I rebased my forward-by-proxy code on top of this branch:
The router pattern '/:id/*' doesn't seem to actually match on urls without a trailing slash, iehttp://localhost:2000/foo, so I think you need to have both patterns present.
The definition let target = item.target in group.js's redirect method happens before item.target is set, which seems to be problematic when I run it locally. Additionally, path seems to convert the url to relative, rather than absolute, which I suspect is not desirable (and definitely doesn't work for proxying the request).

I took a stab at the appropriate fixes here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants