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

loginRequired without Fast Render #80

Closed
ccorcos opened this issue Apr 15, 2015 · 23 comments
Closed

loginRequired without Fast Render #80

ccorcos opened this issue Apr 15, 2015 · 23 comments

Comments

@ccorcos
Copy link
Contributor

ccorcos commented Apr 15, 2015

function requiredLogin(path, next) {
  // this works only because the use of Fast Render
  var redirectPath = (!Meteor.userId())? "/sign-in" : null;
  next(redirectPath);
}

How would we do this without Fast Render?

@Sivli-Embir
Copy link

Not to divert this issue but I found it was better to do this in a global tracker outside of the router.

Tracker.autorun(function () {
  if ( !Meteor.userId() or !Meteor.loggingIn() ) {
    FlowRouter.go("/sign-in")
  }
});

This way any time the user logs out it goes strait to the sign in page, rather then the next time they change routes. Worth noting that this hooks all routes, not just the ones using the middleware.

@arunoda
Copy link
Contributor

arunoda commented Apr 17, 2015

I agree with @Kestanous .
Only way to do it outside of the router.
We are using that too.
Fast Render only helps in the first time. After that, it can't help.

@ccorcos
Copy link
Contributor Author

ccorcos commented Apr 17, 2015

I'm looking at the comment:

this works only because the use of Fast Render

I've noticed that it also works without fast render...

@miri-am
Copy link

miri-am commented Apr 18, 2015

Same for me. I am using

  function requiredLogin(path, next) {
    if(!Meteor.userId()){
      console.warn("redirecting to signin");
      next("/signin");
    }
    else {
      next(); 
    }
  }

and then in all the routes middlewares: [requiredLogin],

Something wrong with that?

@ccorcos
Copy link
Contributor Author

ccorcos commented Apr 18, 2015

Thats what I'm doing too.... just confused about the comment in the Readme

@Streemo
Copy link

Streemo commented May 1, 2015

@ccorcos @miri-am i dont think it is guaranteed to work because the middleware is non-reactive, and logging in in asynchronous. so if you hit the route, and Meteor.loggingIn completes before the middleware is called, then it should be fine. But i dont think that ordering is guaranteed. you should have experienced the other order by now if i understand correctly.

@miri-am
Copy link

miri-am commented May 1, 2015

@Streemo. Yes. I am doing something like the following to rerun the route when logging in is not completed yet...

function redirectBasedOnRole(path, next) {
  if (!Meteor.userId()) {
      console.log("redirecting to signin");
      next("/signin");
  } else {
      console.log("in redirectBasedOnRole else");
      if (Meteor.loggingIn() && Meteor.status().connected) {
          console.log("currently logging in");
          Meteor.setTimeout(function() {
              next(path);
          }, 100);
...

@Streemo
Copy link

Streemo commented May 1, 2015

Hmmm... shouldn't there be a "&& !Meteor.loggingIn()" in if conditional? If
I'm not logged in and I'm not in the process of logging in, the first block
will execute. If I'm not logged in and I am the process of logging in,
Meteor.userId() should still return false and the first block will be
executed anyways. When I'm logged in, Meteor.userId() will return true and
the else condition will be reached, however Meteor.loggingIn() will return
false and the second block will not execute.

I don't see how that code will work without adding the second condition
above to the "if" conditional.
On May 1, 2015 6:36 AM, "Miri" notifications@github.com wrote:

@Streemo https://github.com/Streemo. Yes. I am doing something like the
following to rerun the route when logging in is not completed yet...

function redirectBasedOnRole(path, next) {
if (!Meteor.userId()) {
console.log("redirecting to signin");
next("/signin");
} else {
console.log("in redirectBasedOnRole else");
if (Meteor.loggingIn() && Meteor.status().connected) {
console.log("currently logging in");
Meteor.setTimeout(function() {
next(path);
}, 100);
...


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

@Streemo
Copy link

Streemo commented May 1, 2015

Also I don't think the setTimeout is a strong solution. Outlier cases will
slip past this method. It's not ok to assume that logging in will take a
certain amount of time, because it is unpredictable, based on server load
and network.
On May 1, 2015 10:09 AM, "Jay P" japatel99@gmail.com wrote:

Hmmm... shouldn't there be a "&& !Meteor.loggingIn()" in if conditional?
If I'm not logged in and I'm not in the process of logging in, the first
block will execute. If I'm not logged in and I am the process of logging
in, Meteor.userId() should still return false and the first block will be
executed anyways. When I'm logged in, Meteor.userId() will return true and
the else condition will be reached, however Meteor.loggingIn() will return
false and the second block will not execute.

I don't see how that code will work without adding the second condition
above to the "if" conditional.
On May 1, 2015 6:36 AM, "Miri" notifications@github.com wrote:

@Streemo https://github.com/Streemo. Yes. I am doing something like
the following to rerun the route when logging in is not completed yet...

function redirectBasedOnRole(path, next) {
if (!Meteor.userId()) {
console.log("redirecting to signin");
next("/signin");
} else {
console.log("in redirectBasedOnRole else");
if (Meteor.loggingIn() && Meteor.status().connected) {
console.log("currently logging in");
Meteor.setTimeout(function() {
next(path);
}, 100);
...


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

@ccorcos
Copy link
Contributor Author

ccorcos commented May 3, 2015

i dont think it is guaranteed to work because the middleware is non-reactive, and logging in in asynchronous. so if you hit the route, and Meteor.loggingIn completes before the middleware is called, then it should be fine. But i dont think that ordering is guaranteed. you should have experienced the other order by now if i understand correctly.

I haven't had any problems with this yet... this exchange really ought to happen before the routes are decided though...

@Streemo
Copy link

Streemo commented May 4, 2015

Yeah I feel like login middleware is pretty standard. It's weird that we
have to implement trackers to get it to work safely. Goes against the grain
of FR. I've implemented login logic at the template level.

This moves from having a dedicated login route to having a dedicated login
template, which I render instead using helpers like currentUser. I think
this is better solution as it preserves decoupled reactivity and gives
finer grained control over who sees what on a per template basis.

It doesn't make sense to have flowroutes behave based on something which is
intrinsically reactive; Meteor.user(), else just use iron-router. Instead,
implement this in the template level where reactivity is the law.
On May 3, 2015 7:50 PM, "Chet Corcos" notifications@github.com wrote:

i dont think it is guaranteed to work because the middleware is
non-reactive, and logging in in asynchronous. so if you hit the route, and
Meteor.loggingIn completes before the middleware is called, then it should
be fine. But i dont think that ordering is guaranteed. you should have
experienced the other order by now if i understand correctly.

I haven't had any problems with this yet... this exchange really ought to
happen before the routes are decided though...


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

@vladshcherbin
Copy link
Contributor

@Streemo what are u talking about ? use iron router cuz it can wait for data or check the user ?
The middleware should be reworked into before and after. Besides, we should be able to wait for a data result to show 404, or to check for the user, etc.

This are the main things the router should be able to do. This is not a template layer in any way. And stop pointing the iron router. There are lots of people, not satisfied with its performance with a hope of having another working router.

@arunoda
Copy link
Contributor

arunoda commented May 4, 2015

@vlad we can easily implement the blocking functionality in the router
middlewares. But that's the place where IR sucks and it's the main problem
of it.

If we add it then, we need give more tooling to render loading bars kind of
things from router.

So, what I suggest to do this on the layout level. We will still need more
tooling (outside of the router) to do it. But I hope it's pretty worthy.

I will do a reference implementation next week where we can see how it
works.

My goal for router is to it runs for a change in a router. It register some
stuffs, but it never block for a reactive change or rerun for a reactive
change.

On 2015 මැයි 4, සඳුදා at පෙ.ව. 6.09 Vlad Shcherbin notifications@github.com
wrote:

@Streemo https://github.com/Streemo what are u talking about ? use iron
router cuz it can wait for data or check the user ?
The middleware should be reworked into before and after. Besides, we
should be able to wait for a data result to show 404, or to check for the
user, etc.

This are the main things the router should be able to do. This is not a
template layer in any way. And stop pointing the iron router. There are
lots of people, not satisfied with its performance with a hope of having
another working router.


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

@Streemo
Copy link

Streemo commented May 4, 2015

Hi Vlad, I appreciate your response. But I believe you misunderstood me. I
have no intention of using iron router, it is almost unpredictable and ive
far too often seen too many rerun routes in my app. This is precisely the
reason it makes sense to do it all at the template level. I only say that
if you want reactivity in the router level, then you should use iron router
because that's exactly what they do.

All of what you mention can be done at the template level, there are tools
for it.

Imho the main thing a router should be able to do is take a user to a
requested page. The logic should happen at the view level, because it
decouples it mentally for us.

@Streemo https://github.com/Streemo what are u talking about ? use iron
router cuz it can wait for data or check the user ?
The middleware should be reworked into before and after. Besides, we should
be able to wait for a data result to show 404, or to check for the user,
etc.

This are the main things the router should be able to do. This is not a
template layer in any way. And stop pointing the iron router. There are
lots of people, not satisfied with its performance with a hope of having
another working router.


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

@vladshcherbin
Copy link
Contributor

@arunoda , @Streemo sure, if this can be implemented and easy to write on template layer - it would be nice. Show us the way to simply render the 404 page, login & check the user, do some stuff after the route changed (change the page title, description).

I like this router a lot and use it everywhere instead of IR. This are the things I miss. Just show us the way it can be used.

Thanks

@arunoda
Copy link
Contributor

arunoda commented May 4, 2015

Yep. That's the idea.
I hope we can do this.
After that, I'm going to do a feature freeze and working more on stability.

On Mon, May 4, 2015 at 6:48 AM Vlad Shcherbin notifications@github.com
wrote:

@arunoda https://github.com/arunoda , @Streemo
https://github.com/Streemo sure, if this can be implemented and easy to
write on template layer - it would be nice. Show us the way to simply
render the 404 page, login & check the user, do some stuff after the route
changed (change the page title, description).

I like this router a lot and use it everywhere instead of IR. This are the
things I miss. Just show us the way it can be used.

Thanks


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

@Streemo
Copy link

Streemo commented May 4, 2015

@vlad Assuming you're using flow layout, FlowRouter handles 404s, and you
can use the built in meteor helpers to check validation. I'd like to know
more in detail what you mean, because I've been able to do these things in
template helpers. Is there a more specific way you're referring to?

Thanks
On May 3, 2015 9:18 PM, "Vlad Shcherbin" notifications@github.com wrote:

@arunoda https://github.com/arunoda , @Streemo
https://github.com/Streemo sure, if this can be implemented and easy to
write on template layer - it would be nice. Show us the way to simply
render the 404 page, login & check the user, do some stuff after the route
changed (change the page title, description).

I like this router a lot and use it everywhere instead of IR. This are the
things I miss. Just show us the way it can be used.

Thanks


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

@vladshcherbin
Copy link
Contributor

@Streemo I'm speaking about some example of use cases (when e.g u go /blog/the-post-that-doent-exist), page title and description change, other examples of how it is meant to be used.

@Streemo
Copy link

Streemo commented May 4, 2015

@vlad FR.notFound does this. As for the title and description, I'm not
quite sure.
On May 3, 2015 10:00 PM, "Vlad Shcherbin" notifications@github.com wrote:

@Streemo https://github.com/Streemo I'm speaking of some example of use
cases (when e.g u go /blog/the-post-that-doent-exist), page title and
description change, other examples of how it is meant to be used.


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

@ccorcos
Copy link
Contributor Author

ccorcos commented May 4, 2015

Hmm. Perhaps the router shouldn't run until the login exchange is done... As far as serving a 404 for some missing data, I think this is more of a REST functionality than a web app functionality. The web app just needs to show you a 404 message at the template level. But if you're cURL'ing it, then its more a REST endpoint and should be handled differently...

Just so you know, I'm only using this as a client side router. Its quite convenient to use with React.

@vladshcherbin
Copy link
Contributor

@arunoda Hello, are there any plans on more docs of use cases, like this one with the login. What about the middlewares (before/after) feature?

It's been a long time from the major last changes were made and I fear, this router might be like an IR, with lots of issues and no support. Any news?

@arunoda
Copy link
Contributor

arunoda commented May 18, 2015

We need before/after hooks.
But login stuff needs some other look. I still have to fix some bugs before looking at this.

@arunoda
Copy link
Contributor

arunoda commented Jun 4, 2015

We'll focus on this on #139

@arunoda arunoda added doing and removed question labels Jun 4, 2015
@arunoda arunoda closed this as completed Jun 4, 2015
@arunoda arunoda removed the doing label Jun 4, 2015
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