Exposed initializers for pages are spooky action at a distance and difficult for developers to comprehend

[BrucePerens]

With the advent of authentication, Auth::RequireSignIn is added to BrowserAction. This "exposes" a variable called "current_user", and then the constructor for a page is required to take that as a parameter. The instruction to do that is a needs statement in MainLayout.

Every part of this goes on behind the back of the developer. If the developer changes the requirement for authentication in a page, they have to edit not their own page, but MainLayout, to change needs current_user : User to needs current_user : User? This is sort of "going behind the curtain", the developer didn't write MainLayout and shouldn't have to edit it to do really simple things like turn on and off authentication. The proper thing for the developer to do here would not be to edit MainLayout, anyway, but to create a second base class with the change, so that the first one could still be used in other pages. Ugly, isn't that? This should be a switch in the developer's own page.

It really should be possible for the developer to override the needs statement in MainLayout in their own page, with a needs statement naming a union type with Nil, or even one removing the union type and replacing it with User alone. This should not result in a duplicate variable declaration.

And the entire concept of an action pitching a variable which a page gets, with no text in the developer's own code indicating that this happens, is going to be horribly confusing to the developers. It feels like spooky action at a distance. It is magic that the developer must learn. At a minimum it should have a good deal of guide text, I think while writing the guide text you might gain additional understanding of the difficulty for the developer here. I'd also suggest considering other mechanisms.

[paulcsmith]

Thanks for explaining your thoughts.

I'm not sure if you saw, but there is an authentication guide that goes over optional sign in and skipping sign-in: https://luckyframework.org/guides/authentication/

The idea is that you don't override the current_user, you change it in either MainLayout or create a new layout that doesn't not require a current_user. You can see that in action in src/pages/auth_layout. Those do not require a current_user.

I think this could go into more depth in the guides and I think we could do a better job explaining it. Auth is pretty new so there is definitely some work to do here.

Does reading through the guide help?

[BrucePerens]

Reading through the guides as currently written would not give the developer any idea what unexpose does. I think it gets one mention with no explanation. And the need to go back to a base class to change the authentication for a single page, sorry, is too clunky. Rails gave us this switch in our own action.

[BrucePerens]

One complicating factor is that a logged-out user is Nil. You also have the option to make a logged-out user an unprivileged User object with a logged_in? method returning false. This gets rid of the issue of selecting in the page, rather than the action, whether a user must be logged in or not.

[paulcsmith]

I think there is a misunderstanding with how this works which is probably partly to do with the guides and partly to do with implementation. I'll attempt to explain how it currently is, and in a later issue or code change, make it simpler to use.

There are multiple auth mixins that you can use. See src/actions/mixins/auth. They make it fairly easy to skip sign in, or disallow signed in users completely. You just need to choose the one you want and include it in your action.

Once you've decided that, you need to make sure the page you're using has the correct needs

If you want a page to allow signed in or signed out users, change MainLayout, just once to needs current_user : User?. Then you're good to go.

If you want pages that look significantly different for signed out users, create a new layout and use that one for signed out users.

The idea is different than Rails but doesn't really many extra steps. The only difference is needing to change MainLayout

To make this simpler I may make it so MainLayout works with signed out users and then developers can make it only work for signed in users if they want to. The opposite of what it does now

[BrucePerens]

Well, I get the idea of propagating the type-safeness to the page. I think the best way to clean this up would be to allow the needs statement to override the type declared in a previous needs statement. You could even add a doesnt_need statement.

[BrucePerens]

There is also the question of whether a page should be allowed to ignore an exposed variable.

[paulcsmith]

I see what you're saying. I debated that but it can cause hard to debug issues and confusing logic when you redeclare needs. For example:

class MainLayout
  needs current_user : User

  def render
    # Show the current user name
    h1 @current_user.name
  end
end

class PageWithSignedOutUser < MainLayout
  needs current_user : User?

  def content
  end
end

Now the page is broken because the MainLayout is expecting to output the current_user name, but it might be nil. Crystal would catch it at compile-time, but it just doesn't make sense to redeclare it in cases like this.

That's why it is better to either change MainLayout to accept a nil current_user or to create a new layout.

[paulcsmith]

Pages can't ignore an exposed variable. That's why you need unexpose. Part of the proble you may be seeing is that you should redirect in Home::Index or remove the unexpose. If you want to render the current_user to the page, then you need to expose it

[paulcsmith]

An example here: https://github.com/paulcsmith/crystal-mastery/blob/master/src/pages/main_layout.cr and https://github.com/paulcsmith/crystal-mastery/blob/master/src/pages/guest_layout.cr

You can see I've changed MainLayout so it accepts a user or not: https://github.com/paulcsmith/crystal-mastery/blob/9a302a1aa071785058c6b602afd7ffdd474bf46a/src/pages/main_layout.cr#L6 since the page looks mostly the same

[paulcsmith]

I'm not trying to argue BTW, just making sure we're on the same page before discussing too in-depth

[paulcsmith]

Added this issue as I think it will help: luckyframework/lucky_cli#219

That way you can have a signed in user or no user by default, and if you want to require a user you can make it more restrictive. I think the less-restrictive option is a better default than the current more-restrictive approach

[BrucePerens]

I just want Crystal and Lucky to succeed, and am more concerned about new users and their learning curve and perceptions than what I do in my own code. The main criticism of Rails other than performance is the amount of magic. The main criticism of Ruby other than performance is monkey patching and its potentially global effect. When people get to Lucky, they're going to have bumps to get over, and we want to minimize those so that they don't lose patience and go on to some other language and framework. And we have big ones already like having to figure out what macros are doing when something breaks. This, I think, might be another patience-trier for the new developer.

Regarding redirects, I don't cause extra loads anywhere, if I can avoid it, so would prefer to avoid the redirect. And I write for AMP at the moment, so loading a page - if AMP magic is already resident in the browser - is supposed to be one load.

Now, I could build a switch so that I could control this in my actions, and MainLayout would behave differently depending if I skipped the login requirement in my action or not. That would solve the problem for me. My concern is how to make this easier for the developer. I still think making this decision as easy as possible per page - not per application at generation time - is the best solution.

[paulcsmith]

I agree. I want to make it easier and I think it is too complex now, but I'm not clear where the stumbling block is. What exactly are you try to do in your Home::Index page and your IndexPage? Not with code, but in general user terms? I'm not sure I understand what the tricky part is in this context. I think it would basically be removing unexpose and change needs in the MainLayout but I'm not sure of the use-case so I can't understand how to improve it yet

[BrucePerens]

I should be able to say, as tersely as possible, that a specific action requires authentication, or not. The use case is that there are public pages to the web site, and there is the capacity to comment or edit. Public pages should not require a login, and should get indexed by search engines. Edits and comments should require a login. And you have satisfied this adequately for the action by providing a module to include that eschews authentication.

This should propagate to the page in a way that is as simple as possible for the developer. This is where we stumble.

[paulcsmith]

Ok I think I get it now. In that case, the modules should work for the actions (as you said)

For the pages, all you should need to do is change needs current_user : User in MainLayout to needs current_user : User? If you still had a compile-time error then that is a problem. If it was the case, my guess is that it was with unexpose and all you need to do is remove that.

If none of those options work, can you tell me what the error was? Because it should be that simple and if it's not I definitely want to improve/fix that

[paulcsmith]

I'll also make it so you don't need unexpose in the Home::Index in a default Lucky project. I've figured out a way around that. I think that might help

[paulcsmith]

I'll also document those modules better so people know what they can do

[BrucePerens]

Maybe we need some help for the newbie in a macro. Consider that I'm a newbie. I have an application that is globally set up to require authentication, as at present. I add the skip module to the action. I go to build, and the page now fails to build with a completely cryptic error regarding the constructor.

So, we fix the macro to emit more help, but the help says not to edit the developer's page, but to edit MainLayout. The developer is still in the dark about why this happened. If the help said what to do to the developer's own page, it would make more sense to the newbie.

[paulcsmith]

I like how you stated that from a newbie perspective. I think it should work better by making current_user optional in a default Lucky application. That will make it "just work" when you add the module.

If you want to make the type more restrictive you can modify the MainLayout. This should be good because it will work with the newbie, will remove the weird constructor errors as well.

I personally don't feel the MainLayout needs is magic in the traditional sense. It uses the same macro as every other page. The tricky part is that the user didn't make it.

We could make it so that the user declares needs current_user : TheDesiredType in every page, but that is a pain. That's why the needs current_user is in the layout, because otherwise it would be a huge pain to cover in every single page.

So just to be clear. Here is the new way a newbie would approach this:

class Posts::Index < BrowserAction
  include Auth::SkipRequireSignIn
  # Code
end

# Just works because MainLayout can accept a nil current_user
class Posts::IndexPage < MainLayout
end

So now newer developers can use the modules as they like without changing their pages or layouts at all.

If later on, the user decides "hey, I only ever show this layout when there is a user present. Dealing with nil is annoying" They can then (ask for help/check the guides) and make needs current_user : User so that you can only use the layout with a signed in user.

I think this makes the most sense. New users can use things right away with minimal changes. Then if you get more advanced and you have an app where only signed in users are allowed (think Gmail, SalesForce, etc.) then you can restrict the type. Even if you don't restrict the type, the app will still work. So I think this is a much better way to approach it.

I'll update the generated app: luckyframework/lucky_cli#219

And update the guides to make this more clear and give more examples of how to use the built-in modules

[BrucePerens]

That will reduce pain for the newbie, thank you. I think we can then explain somewhere how the developer can enforce login safety in the page by writing StrictLoginMainLayout. There is no opposite case, a page that doesn't use current_user is insensitive to whether it is passed or not.

[paulcsmith]

That makes sense. Thanks for helping walk through this. I think this will be a much better default experience for people! I'll close this when the changes to the website and generated app are done

[BrucePerens]

The developer's code will be slightly complicated because type restriction using .is_a? doesn't work on class variables, for safety. So, anywhere the developer calls methods of User in a page, it has to be passed to a local variable first:

if (u = @current_user).is_a?(User)
   # Call methods of u
end

I guess we will have to teach them that form. This would be unnecessary if the logged-out user was an unprivileged User object, rather than Nil.

[paulcsmith]

@BrucePerens This is true, but it would be true no matter what since instance vars don't work well with union types (as you mentioned). I don't think there is a way around that, though I'd love it if Crystal could somehow make this work without having to do these workarounds. I think it's one of the more confusing parts of Crystal. No one gets it at first. With that said there are few ways to make this read a bit better:

# I like to use `try` if I don't need an else statement
@current_user.try do |user|
  div do
    text user.name
  end
end

# or if you just need something short
text @current_user.try(&.name) || "No name"

You also don't need to do is_a? if you assign to a local var:

user = @current_user
if user
  text user.name
else
  text "Guest
end

I hope that helps a bit. Though I'd love it if there were no shared memory so the compiler could do this without assigning to a local var :D I don't have the time or expertise to implement that in Crystal though

[BrucePerens]

You could figure out how to pass the exposed variables by value, on the argument list. Then they wouldn't be volatile.

[paulcsmith]

I'm not sure I understand. Since the page is a class, the only way to pass instantiate data with it is with an instance variable, so if it is nil you would always need to check it. The only other way would be to change how needs and expose works and instead pass them as arguments to render. That would be even more painful though since you'd need to add every argument needed to render and it would make extracting some private methods more verbose since you'd have to pass everything around as an argument.

But maybe I misunderstood what you're saying. Could you give an example?

[BrucePerens]

I can't come up with a cleaner way to pass the variables as arguments. So, never mind.

[paulcsmith]

Closed by #667. Also going to do more in luckyframework/lucky_cli#219 to clarify things