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

onRoute hook #155

Closed
davidguttman opened this issue Jul 10, 2016 · 14 comments
Closed

onRoute hook #155

davidguttman opened this issue Jul 10, 2016 · 14 comments

Comments

@davidguttman
Copy link

For certain things like authenticated routes, it would be useful to have a function that runs before each route change so that authentication can be checked and the user can be redirected. Something like:

const app = choo({
  onRoute (route, state, prev, send) {
    if (route.auth && !state.authenticated) {
      return send('location:setLocation', { location: '/login' }) //redirect
    }

    // otherwise default behavior
  }
})

app.router((route) => [
  route('/login', { auth: false }, loginView),
  route('/protected', { auth: true }, protectedView)
])
app.start()
@yoshuawuyts
Copy link
Member

as discussed on IRC, def sounds like a useful addition

@timwis
Copy link
Member

timwis commented Jul 10, 2016

Cool idea. Do you think it provides value over just wrapping your routes? ie.

const auth = (view) => (state, prev, send) => {
  return state.authenticated ? view(state, prev, send) : loginView(state, prev, send)
}

app.router((route => [
  route('/', homeView),
  route('/account', auth(accountView)),
  route('/purchases', auth(purchasesView))
])

@davidguttman
Copy link
Author

davidguttman commented Jul 10, 2016

@timwis that's a cool way of doing it, didn't think of that.

Another related feature that onRoute is useful for is changing the page title on a route change:

const app = choo({
  onRoute (route, state, prev, send) {
    if (route.auth && !state.authenticated) {
      return send('location:setLocation', { location: '/login' }) //redirect
    }

    document.title = 'AppName - ' + route.title // could be cool to incorporate params too...
    // default behavior
  }
})

app.router((route) => [
  route('/login', { auth: false, title: 'Login' }, loginView),
  route('/protected', { auth: true, title: 'Protected' }, protectedView)
])
app.start()

Not sure what the best way with the wrap approach would be.

@timwis
Copy link
Member

timwis commented Jul 10, 2016

Yeah that would be handy. I imagine that could be done in the view's onload callback, though it'd be annoying to have to do that for every view.

@mantoni
Copy link
Contributor

mantoni commented Jul 12, 2016

I was just about to create a new issue, but I think my case is similar enough to this one:

Basically I want to initialize the model with an asynchronous call (e.g. XHR). I played around with an onload handler, but ended up creating a subscription that calls a reducer with the data and passed on done.

I can't tell whether there's already a better / more clear way to do this or if that's something that could be addressed with this feature?

@yoshuawuyts
Copy link
Member

@mantoni Hmmm, the way I view this is:

  • If it's a single view that calls a specific endpoint once it's loaded, the onload hook should be the way to go; probably calling an effect in the process.
  • If every view is calling endpoints when they start up, creating an abstraction might make sense.

@mantoni
Copy link
Contributor

mantoni commented Jul 12, 2016

@yoshuawuyts Can you explain where you see this being different in the onRoute case? I understand that the case being made here could also be solved in onload?

@mantoni
Copy link
Contributor

mantoni commented Jul 12, 2016

@yoshuawuyts Sorry, ignore me. I just saw that onRoute is a feature of the choo function. I was confusing it with a model feature. It's all clear now. Thanks.

@yoshuawuyts
Copy link
Member

Hmmm, though on the other hand... this might make choo a bit more frameworky - less hard to figure out what to expect. Maybe we should not make it this way? Or maybe we should call it wrapView() and put it in line with #145 ? idk - not quite sure yet what the best way of approaching this is

Glad the solution @timwis proposed should at least provide a workable experience right now :)

@davidguttman
Copy link
Author

For discussion, here's another use case: logging. In my apps I'll often want the browser to tell the server which routes/params are being used.

@yoshuawuyts
Copy link
Member

@davidguttman hmm, that should already be possible with the onState() hook, as the current path is simply a value on the state

@saem
Copy link

saem commented Aug 20, 2016

Came here based on the use case of a header marking a nav element as current, or active, based on where one was in the app. I could read it based on app.state.location, after I clean it up, or it'd be neat if I could just listen to it as a effects, perhaps?

(Edit: I meant to write effects instead of subscriptions, currently namespacing doesn't allow this AFAICS)

@thomhos
Copy link

thomhos commented Sep 17, 2016

Hey @saem,

I also spent some time figuring out how to set my active menu item. I kept getting into situations where I changed the state and triggering an infinite loop.

I'm using a sidebar and only want to render the content in the area next to it, to do that I was already wrapping my views in a function like @timwis did with the auth example. It was then a matter of passing also the route name to that function and do another check.

  app.router('/', (route) => [
    route('/', subview(views.home, '/')),
    route('/about', subview(views.articles, '/about'))
  ]);

const subview = (view, route) => (state, prev, send) => {
    const { activeRoute } = state.sidebar;

    if (route !== activeRoute) {
      send('sidebar:setActiveRoute', route);
    };

    return views.main(state, prev, send, view)
  };

I have a model for my sidebar which keeps track of the active route, menu items and whether or not it's expanded (mobile).

It seems to do the job pretty good but I'm sure there will be a more elegant way of doing this. If anyone has one please shout out.

Cheers!

@yoshuawuyts
Copy link
Member

I think this question is answered. Thanks for opening!

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

6 participants