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

Improved Middlewares #59

Merged
merged 1 commit into from
Jun 4, 2015
Merged

Improved Middlewares #59

merged 1 commit into from
Jun 4, 2015

Conversation

arunoda
Copy link
Contributor

@arunoda arunoda commented Apr 2, 2015

Read this first #57

Middlewares are pretty important and we can have some good API for middlewares. In this PR. We can have these.

  • after and before middlewares (may be we can rename it as hooks)
  • middleware gives context as the first argument
  • With the named params, we can implement the except and only functionality
  • they can't run any reactive content inside and wait the router

Now our middlewares seems not like middlewares in express/connect because we don't have a way to wait. Our middlewares seems not like IR hooks since there won't be any reactive content. So, using middleware or hooks confuse the user and think we inherit some of their features. That's why we introduce a new name triggers

API

For individual routes

FlowRouter.route('/home', {
  // just like middlewares we are having right now
  triggersEnter: [trackRouteEntry],
  action: function() {

  },
  // calls when when we decide to move to another route
  // but calls before the current current started
  // we can't implement this in `FlowRouter.go` since it's possible to 
  // navigate to this route by clicking a link
  // So use page.exit: https://goo.gl/ubAKF3
  triggersExit: [trackRouteClose]
});

function trackRouteEntry(context) {
  Mixpanel.track("visit-to-home", context.queryParams);
}

function trackRouteClose(context) {
  Mixpanel.track("move-from-home", context.queryParams);
}

For all routes

FlowRouter.triggers.enter([cb1, cb2]);
FlowRouter.triggers.enter([trackRouteEntry], {only: ["home"]});
FlowRouter.triggers.exit([trackRouteEntry], {except: ["home"]});

We can't have both only and except at once.

@zimt28
Copy link

zimt28 commented Apr 8, 2015

Cool! So what I'd like to to with middleware/ hooks is

  1. Prefix routes with a language code. If there is one, read it, if there is none, set it. (Maybe also hook into the url generation stuff to prefix generated urls. But that's optional I think).
    This requires some kind of "route rewrite". So /en/about would have to use the registered /about route.
  2. Show loading pages. I like the idea of showing data as it arrives, but that's not ideal for every app. It's easy to implement a loading screen with Tracker.autorun and FlowRouter.subsReady, but it's bad having to manually code it for every route. A hook would be nicer!

@arunoda
Copy link
Contributor Author

arunoda commented Apr 21, 2015

In the next version of middlewares, I am trying to run all middlewares in the same eventloop. That means, you can't use "next()" asynchronously and wait the router.

If you don't call "next()" we will call it after the middleware function runs.

This is a step to move all the logic into template layer or something else. But not in the router.

So, now biggest question will be how to deal with login checks and so on. I am planning implement a basic set of Blaze helpers and a set of Apis to do it in the template layer. It's won't be something in the router. It's a seperate package.

@arunoda
Copy link
Contributor Author

arunoda commented Apr 21, 2015

@zimt28 for your second point, I don't think router need to implement it.
It's something on the template layer. It can be added an a set of helpers or react mixins.
That's the way to go.

@delgermurun
Copy link
Contributor

@arunoda Have you already began working on it? As always, I'm willing to help.

@arunoda
Copy link
Contributor Author

arunoda commented Apr 21, 2015

Sure. Thanks. Just brainstorming here. Didn't start. I'll make an open
document before starting the work.

On Tue, Apr 21, 2015 at 10:23 AM Delgermurun Purevkhuu <
notifications@github.com> wrote:

@arunoda https://github.com/arunoda Have you already began working on
it? As always, I'm willing to help.


Reply to this email directly or view it on GitHub
#59 (comment)
.

@delgermurun
Copy link
Contributor

Document is good idea.

@delgermurun
Copy link
Contributor

Let's do it.

@arunoda Can you write document? Including API syntax etc...

If you permit, I would like to implement the proposal :).

@arunoda
Copy link
Contributor Author

arunoda commented May 27, 2015

Okay sure. I will do it in few hours :)
I rename middleware as triggers.
On 2015 මැයි 27, බදාදා at පෙ.ව. 8.57 Delgermurun Purevkhuu <
notifications@github.com> wrote:

Let's do it.

@arunoda https://github.com/arunoda Can you write document? Including
API syntax etc...

If you permit, I would like to implement the proposal :).


Reply to this email directly or view it on GitHub
#59 (comment)
.

@delgermurun
Copy link
Contributor

@arunoda Please rebase this branch as well.

@vladshcherbin
Copy link
Contributor

yeeeey, today is a great day. with the before/after middleware, the router will be a complete replacement for IR.

@arunoda just a proposal, maybe we can stick with middleware name, in many frameworks it is named so. triggers are kind of db stuff ;)

@arunoda
Copy link
Contributor Author

arunoda commented May 27, 2015

I'm okay with any name.
With the new proposal we don't have callback called next or done in the
middleware.
They run before and after the router. But without waiting.

If we call it middlewares, people except it to be work like a middleware in
express/connect.
That's why I put this name.

On Wed, May 27, 2015 at 9:18 AM Vlad Shcherbin notifications@github.com
wrote:

yeeeey, today is a great day. with the before/after middleware, the router
will be a complete replacement for IR.

@arunoda https://github.com/arunoda just a proposal, maybe we can stick
with middleware name, in many frameworks it is named so. triggers are kind
of db stuff ;)


Reply to this email directly or view it on GitHub
#59 (comment)
.

@vladshcherbin
Copy link
Contributor

@arunoda sure, any name would be nice then. Can't wait to see the new api and usage examples.

Thank you guys for you work!

@arunoda arunoda force-pushed the improved-middlewares branch from c8865c9 to 78dbfa9 Compare May 27, 2015 05:00
@arunoda
Copy link
Contributor Author

arunoda commented May 27, 2015

@delgermurun check the PR description. I updated how the API should looks like.
We might can improve the naming, just focus on the functionality for now.

@delgermurun
Copy link
Contributor

@arunoda Thanks. I'll do prototype shortly.

@arunoda
Copy link
Contributor Author

arunoda commented May 27, 2015

Awesome.

@arunoda arunoda merged commit 78dbfa9 into master Jun 4, 2015
@arunoda
Copy link
Contributor Author

arunoda commented Jun 4, 2015

Triggers are alive: https://github.com/meteorhacks/flow-router#triggers
(version 1.10.0)

@cstrat
Copy link

cstrat commented Jun 5, 2015

I just updated and am trying to implement triggers.
You've mentioned removing the login validation logic out of the router...

Doesn't it make sense though to have that in there?
I loved how I could group routes, and apply some logic to check if a user was logged in, if so, they can access these routes... check if a user is an admin, they can see these routes.

If this moves into template logic, that means I need to repeat myself with each template... I guess I can wait to see how it plays out.

By the way, thanks for the great work on everything you've contributed!

@lfades
Copy link

lfades commented Jun 10, 2015

Arunoda, I have middlewares like this

middlewares: [function (path, next) {
  next(!Groups.can('see_something') && '/home');
}]

What do I do now?

@arunoda
Copy link
Contributor Author

arunoda commented Jun 10, 2015

See: #153

@acomito
Copy link

acomito commented May 19, 2016

is it possible to feed the 'name' of a route group into the 'except' array?

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

Successfully merging this pull request may close these issues.

7 participants