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

Support dynamic redirect destinations #57

Closed
kmjennison opened this issue Jan 26, 2021 · 7 comments
Closed

Support dynamic redirect destinations #57

kmjennison opened this issue Jan 26, 2021 · 7 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@kmjennison
Copy link
Contributor

Is your feature request related to a problem? Please describe.
As mentioned in the limitations, there isn't built-in support for custom redirect destinations based on the user's auth status. This limits the usefulness of the redirects.

Describe the solution you'd like and how you'd implement it
Allow users to provide functions for the authPageURL and appPageURL config properties that should return the appropriate URL. We could pass either ctx or window to the function, depending on the context.

// ctx would be defined when server-side redirecting, window for client-side
({ ctx, window }) => {
  // some logic
  return '/my-auth-page?next=/my-app/'
}

Is this a breaking change?
No. We would still support static values for the config options.

Describe alternatives you've considered
None

@tlays
Copy link
Contributor

tlays commented Jan 30, 2021

I like this idea a lot. It does provide flexibility both at CSR and SSR.

Just a few comments / observations:

  • You mention it's for the authPageURL and appPageURL config properties, but I assume it would also work for the option parameters of all withAuthXXX functions right ? I mean, it has too.

  • I believe it's implied, but just for clarity, authPageURL and appPageURL could be provided either as string, or functions right ?

  • I'm not sure that passing window in parameters is a good idea. At CSR it's globally available anyway. The only time I tried to access window like this through a parameter my code did not behave the way I expected it to. Simply using the global window object after checking it exist (ex: if (typeof window !== 'undefined')) was the best/safest option.

  • Shouldn't we expose the AuthUser also ? I know it's available in ctx, but maybe making it explicitly available as a parameter would be easier to understand

// ctx would be defined when server-side redirecting
// window is globally available for client-side
// AuthUser is available for custom logic to be built around
({ ctx, AuthUser }) => {
  // handle CSR vs SSR
  if (typeof window !== 'undefined')
    // do CSR stuff here
  else if (ctx)
    // do SSR stuff here

  // some logic
  return '/my-auth-page?next=/my-app/'
}

@kmjennison
Copy link
Contributor Author

@tlays Thanks for the thoughtful feedback! I agree with all of your suggestions.

You mention it's for the authPageURL and appPageURL config properties, but I assume it would also work for the option parameters of all withAuthXXX functions right ?

Yes

I believe it's implied, but just for clarity, authPageURL and appPageURL could be provided either as string, or functions right ?

Yes

I'm not sure that passing window in parameters is a good idea.

I agree

Shouldn't we expose the AuthUser also ?

I agree, this is a good idea

@kmjennison
Copy link
Contributor Author

After this is implemented, we should also replace the "custom redirect" example added in #51. A simple demo would be to include the user's email in a URL parameter value when redirecting.

@kmjennison kmjennison added the help wanted Extra attention is needed label Feb 4, 2021
@tlays
Copy link
Contributor

tlays commented Feb 9, 2021

After this is implemented, we should also replace the "custom redirect" example added in #51. A simple demo would be to include the user's email in a URL parameter value when redirecting.

Yes, or maybe add another (a new) example page specific for this redirect technique (with a dynamic redirect at CSR vs SSR). IMO, the example in #51 is still interesting to keep in order to illustrate how to use the built-in / standard SSR support for notFound and redirect.

@SelfDevTV
Copy link

SelfDevTV commented Mar 22, 2021

Any ETA on this awesome feature? :)

Right now I do it like this (maybe that helps others):

getServerSideProps

export const getServerSideProps = withAuthUserTokenSSR()(async (props) => {
  if (!props.AuthUser.id) {
    return {};
  }
  return {
    redirect: {
      destination: "/cockpit",
      permanent: false,
    },
  };
});

useEffect - I have this in _app.js so it works on every page

useEffect(() => {
    firebase.auth().onAuthStateChanged((user) => {
      if (user) {
        router.push("/cockpit");
      } else {
        router.push("/");
      }
    });
  }, []);

@kmjennison
Copy link
Contributor Author

Closed by #121 and released in v0.13.0-alpha.3.

@kmjennison
Copy link
Contributor Author

Opened an issue to update the documentation and example: #122

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants