Skip to content
This repository has been archived by the owner on Jan 11, 2023. It is now read-only.

Support server side goto? #83

Closed
neonquill opened this issue Jan 13, 2018 · 6 comments · Fixed by #127
Closed

Support server side goto? #83

neonquill opened this issue Jan 13, 2018 · 6 comments · Fixed by #127

Comments

@neonquill
Copy link

I'm running into problems trying to implement a login system where the user gets automatically redirected to a login page when first loading the site. For example:

<:Head>
  <title>Hello world!</title>
</:Head>

<Layout page='home'>
  <h1>Hello world!</h1>
</Layout>

<script>
 import { goto } from 'sapper/runtime';
 import Layout from './_components/Layout.html';
 import auth from './_auth';

 export default {
   components: {
     Layout,
   },

   preload() {
     if (!auth.userLoggedIn()) {
       goto('/login');
     }
   },
 };
</script>

This pattern works fine as long as preload() runs on the client. If this is the first load and this code gets run on the server then I get a "URL is not defined" error inside the goto() implementation.

Looking at the code it seems like goto() was never designed to be used server side. I was wondering if I'm missing something fundamental about the sapper architecture and I should be using a completely different pattern to approach this, or if this is just a limitation of the current implementation.

@EmilTholin
Copy link
Member

EmilTholin commented Jan 13, 2018

Hi @neonquill! Since sapper is just an Express middleware, maybe you could handle this redirect on the server?

app.get('/', function(req, res, next) {
  const isNotLoggedIn = req.user === undefined;
  if (isNotLoggedIn) {
    res.redirect('/login');
  } else {
    next();
  }
});

app.use(sapper());

@neonquill
Copy link
Author

@EmilTholin - The biggest challenge with that approach is that I basically want every single page to redirect the user to the login page, not just '/'. If I attempt to wildcard the route too broadly I end up catching lots of files I have no intention of redirecting. I guess sapper should have a list of all the valid routes somewhere that I could pull out and pass to express.

The other issue with this approach is that it still requires implementing the redirect independently on the client and the server. Not really a big deal, just one more place for me to make mistakes.

As an aside, I was paying closer attention to the other open issues, and it feels like a fix to #75 would also solve my problem. If I could return a redirect from preload() then I could write the code in one place to call goto() if I'm running in the client and return a redirect if I'm on the server.

Regardless, I'm happy with the answer that sapper wasn't designed to support calling goto() inside the preload() function and I need to find another workaround. Feel free to close if that's the bottom line.

@johanalkstal
Copy link

I was just trying the exact same thing as @neonquill (great minds think alike).

Handling it as another Express middleware would not have been an obvious idea for me.
This because Sapper has server routes, so my instincts say that all server routing would go there.

I think there's a mindset that needs to be painted here.
What parts are for the client?
What parts are for the server?
What parts are handled by Sapper?
What parts are handled by Express?

The lines are a bit blurry.

@Rich-Harris
Copy link
Member

Maybe we could solve this by adding a context to preload with methods like redirect and error:

import * as auth from './_auth.js';

export default {
  preload: ({ params, session, query }) => {
    // if `session` exists, we're on the server
    const user = session ? session.user : auth.user;

    // if we're not logged in, redirect
    if (!user) {
      this.redirect('/login');
    }

    else if (isInvalidForSomeReason(params.whatever)) {
      this.error(404, `Could not find ${params.whatever}`);
    }

    else {
      return fetch(`/api/things/${whatever}`).then(r => r.json()).then(result => ({ thing: result }));
    }
  }
};

@johanalkstal
Copy link

Yeah that could work!

@ansarizafar
Copy link

That's how it should be done.

Rich-Harris added a commit that referenced this issue Feb 18, 2018
implement this.redirect in preload
Rich-Harris added a commit that referenced this issue Feb 18, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants