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

Documentation - Authentication improvement proposal #107

Closed
pierreyves-lebrun opened this issue Jun 29, 2019 · 7 comments
Closed

Documentation - Authentication improvement proposal #107

pierreyves-lebrun opened this issue Jun 29, 2019 · 7 comments

Comments

@pierreyves-lebrun
Copy link

At first let me tell you that I think you made a truly amazing job with this library, I prefer it to React router by far.

While I was playing around I encountered the issue you are describing under the docs Delaying render until authentication section and while you explained one possible way to fix it, I thought I would share the one I found.

I basically didn't like too much the idea of rendering the router before knowing the authentication state and felt like passing an isAuthenticationStateKnown prop to the context was a bit hacky as to me routes shouldn't be aware of the authentication state at all.

Instead I came up with the following solution:

export default function App() {
  // Custom hook that handles current user authentication state
  const [currentUser, setCurrentUser, authenticationStateIsUnknown] = useAuthentication()

  // Don't render anything until we know user authentication state
  // Could instead render a spinner or something indicating that the app is loading here
  if (authenticationStateIsUnknown) {
    return null
  }

  return (
    <Router routes={routes} context={{ currentUser, setCurrentUser }}>
      <Suspense fallback={null}>
        <View />
      </Suspense>
    </Router>
  )
}

As you can see, the useAuthentication() custom hook returns a authenticationStateIsUnknown boolean which we can use to determine whether to render the router or not.

I won't share my custom hook code here as it is too business specific but its implementation is fairly trivial.

Feel free to tell me what you think of this solution as I might be disregarding concerns that come along with it. I thought the docs should probably show a self-contained authentication example that doesn't require the routes to handle any transitional state.

@mrtnbroder
Copy link

mrtnbroder commented Jun 30, 2019

How about you just wait on the initial state, and then render?

const getContext = async (): Promise<RoutingContext> => {
  // try fetching current user if authenticated
  const account = await auth.getCurrentUser()
  return {
    account,
  }
}

export const main = async () => {
  const rootEl = document.getElementById("root")
  const renderer = process.env.NODE_ENV === "production" ? hydrate : render
  // suspend render until context is ready
  const context = await getContext()
  const navigation = createBrowserNavigation({
    routes,
    context,
  })

  // Wait until the navigation has loaded the page's content, or failed to do
  // so. If you want to load other data in parallel while the initial page is
  // loading, make sure to start loading before this line.
  await navigation.getRoute()

  renderer(<App navigation={navigation} />, rootEl)
}

@jamesknelson
Copy link
Collaborator

@pierreyves-lebrun thanks for writing this down -- sorry about the slow reply!

I think this could be a good way of doing things when you're working within a single page app with no SSR. However, the way I'm moving with Navi is that I want it to work out of the box with SSR, in which case you actually do need to render stuff before the authentication state is unknown.

For example, here's how I handle this in an app I'm building at the moment with SSR and client-side auth:

async function main() {
  let backend = new Backend()
  let context = {
    backend,
    currentUser: undefined,
  }
  let navigation = createBrowserNavigation({
    routes,
    context,
  })

  function updateContext(change) {
    context = { ...context, ...change }
    navigation.setContext(context)
  }

  await navigation.getRoute()

  ReactDOM.hydrate(
    <BackendContext.Provider value={backend}>
      <Router navigation={navigation} />
    </BackendContext.Provider>,
    document.getElementById('root'),
  )

  let currentUser = await backend.currentUser.getCurrentValue()
  if (currentUser !== undefined) {
    updateContext({ currentUser })
  }
  backend.currentUser.subscribe(currentUser => {
    updateContext({ currentUser })
  })
}

It's not pretty, and I'd definitely like to find a better way of doing this. But so far, this seems to be the approach that's resulted in the least amount of unnecessary re-renders, and thus the fastest loads.

@mrtnbroder
Copy link

@jamesknelson , I don't understand. Why do you have to render without a user first? This has nothing to do with SSR. You pass data into the context, and the initial context should have all the information about the user already. Instead of rendering the app first without the right context, wait for the backend to return a user (or not) and then render. What's the issue here?

@jamesknelson
Copy link
Collaborator

The issue for me is that I want to cache pages so that they can be sent to people who are both logged in and logged out, and then add the authentication state once the page has loaded. Otherwise it becomes impossible to put generated pages on an edge cache.

As a result, the pre-rendered HTML assumes the user is not authenticated, and thus the initial ReactDOM.hydrate needs to match that. Then, the auth state can be added once the page is hydrated.

@mrtnbroder
Copy link

Hm, why do you think caching pages that require authentication is something worthwhile? Because, I think it causes more problems then it solves. Today's SPA's are blessed with Code-Splitting, so I don't think performance is an issue here.

@jamesknelson
Copy link
Collaborator

For example, if you're showing the current user in the top right hand side of the screen, but the rest of the page is static content, then the page is inherently cacheable other than the authentication widget. If you're rendering using a serverless function on ZEIT now for example, then taking into account function spin-up time and lag to wherever the requesting user is, the cache can turn a 1-2s request into 50ms.

Of course, this depends on what kind of content you're serving, but I feel it can be well worth it.

@pierreyves-lebrun
Copy link
Author

Thanks to both of you, your answers have been instructive.

I personally am still relatively new to react and never played with SSR so far. I’ll try your solutions when I reach that point.

In any case, please keep up the good work!

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

No branches or pull requests

3 participants