Middleware: Pass non-branch query parameters on redirect#81
Merged
Conversation
sirreal
reviewed
Dec 19, 2018
Member
sirreal
left a comment
sirreal
left a comment
There was a problem hiding this comment.
Thanks for looking at this! I left a question about encoding.
I always have difficulty testing changes to this codebase, so I don't think I can help much there.
|
|
||
| const query = | ||
| Object.keys( req.query ) | ||
| .reduce( ( q, key ) => key === 'branch' ? q : q.concat( `${ key }=${ q[ key as keyof {} ] }` ), [] ) |
Member
There was a problem hiding this comment.
Express seems to decode the query:
https://expressjs.com/en/api.html#req.query
Should we passing key and value through encodeURIComponent to build the URL below?
Member
Author
There was a problem hiding this comment.
sounds like a good catch there @sirreal and yah I agree - let's encode them. I'm AFK now for a couple weeks so feel free to commandeer the PR.
Resolves #79 When we remap a given URL from `calypso.live?branch=...` to the "subdomain" URL we have been stripping away the entire query string when redirecting the browser. This causes problems when we want to pass flags to Calypso or otherwise send query args. In this change we're appending the existing query string without the branch name specified so that we can pass everything else through. No query arg named `branch` will pass through though.
sirreal
force-pushed
the
update/pass-query-params
branch
from
ca281a4 to
f6ca587
Compare
sirreal
approved these changes
Jan 12, 2019
Member
sirreal
left a comment
sirreal
left a comment
There was a problem hiding this comment.
I added encoding for the query keys and values, this looks like it should fix.
I'll try landing this over the weekend.
sirreal
added a commit
that referenced
this pull request
Jan 12, 2019
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves #79
When we remap a given URL from
calypso.live?branch=...to the"subdomain" URL we have been stripping away the entire query string when
redirecting the browser. This causes problems when we want to pass flags
to Calypso or otherwise send query args.
In this change we're appending the existing query string without the
branch name specified so that we can pass everything else through.
No query arg named
branchwill pass through though.Testing
I need help testing this. I think the local environment is different than the
production one with subdomains and so I don't know if I can get this flow
fully tested locally.