Skip to content

Implement dynamic redirect (#624) #634

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

Merged
merged 7 commits into from
Sep 13, 2016
Merged

Implement dynamic redirect (#624) #634

merged 7 commits into from
Sep 13, 2016

Conversation

fnlctrl
Copy link
Member

@fnlctrl fnlctrl commented Sep 1, 2016

With e2e tests

// 1. resolve relative redirect
const rawPath = resolveRecordPath(redirect, record)
const rawPath = resolveRecordPath(typeof redirect === 'function' ? redirect() : redirect, record)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we cache type of redirect === 'function'?
I didn't because I thought it was too trivial.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't have an impact on performance so I think it's ok this way.

However, I think it's better to call redirect if it's a function and let the rest of the code the same:

redirect = typeof redirect === 'function' ? redirect() : redirect

This way the return of redirect can also be an object with a name and it's more flexible

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep! You're right, fixing it now

@LinusBorg LinusBorg added the 2.x label Sep 5, 2016
@yyx990803
Copy link
Member

Shouldn't the redirect function have access to the target route to be actually useful?

@@ -52,7 +52,8 @@ export function createMatcher (routes: Array<RouteConfig>): Matcher {
location: Location
): Route {
const { query, hash, params } = location
const { redirect } = record
const { redirect: originalRedirect } = record
const redirect = typeof originalRedirect === 'function' ? originalRedirect() : originalRedirect
Copy link
Member Author

@fnlctrl fnlctrl Sep 8, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yyx990803 Sure it would be useful, but the target route is match()-ed after consuming the redirect option... and this context is not available here, so I don't know what can be passed to the redirect function...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can pass createRoute(record, location)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that makes sense! On it now.

@yyx990803
Copy link
Member

Ideally also need a test case for the to argument ;)

@fnlctrl
Copy link
Member Author

fnlctrl commented Sep 8, 2016

Right.. I'll get back to it after dinner 😄

@fnlctrl
Copy link
Member Author

fnlctrl commented Sep 8, 2016

http://localhost:8080/__build__/shared.js
Failed to load resource: the server responded with a status of 404 (Not Found)
http://localhost:8080/__build__/auth-flow.js 
Failed to load resource: the server responded with a status of 404 (Not Found)

Strange that I can't seem to run e2e tests on the current PC I'm using (was running fine on another one), npm run dev has the same error too...
Tried npm cache clear npm install many times still no luck..

Using CI for debug now.. sigh

@fnlctrl
Copy link
Member Author

fnlctrl commented Sep 8, 2016

Note:
Stuck on removing hash and query after redirect

@posva
Copy link
Member

posva commented Sep 8, 2016

Apart from tests failing LGTM

@yyx990803
Copy link
Member

Merging as-is and fixing it later

@yyx990803 yyx990803 merged commit 6475d9b into next Sep 13, 2016
@yyx990803 yyx990803 deleted the dynamic-redirect branch September 19, 2016 21:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants