Remove needs from HTMLPage module

[matthewmcgarvey]

The intention behind "needs" was to allow for passing data down into html pages without having to do it in every place we render each page. Things like current_user would be a pain to add everytime you render a page and there would be a ton of places to update if you need to add something or refactor in another way. So needs was added to abstract away the way we "new up" an object while expose was added to include data into this "newing up" process.

class ShoppingCart::Show < BrowserAction
  expose shopping_cart

  get "/shopping_cart" do
    html ShowPage
  end

  def shopping_cart
    ShoppingCartQuery.find(shopping_cart_id)
  end
end

class ShoppingCart::ShowPage < MainLayout
  needs shopping_cart : ShoppingCart
  # ...
end

Behind the scenes, expose is adding that reference to a global list of exposed data points and the html macro is newing up the page and passing in all exposed data points while the page takes what it needs from all the things passed in and ignores the rest.

That all kind of sounds nice, but that's a lot of infrastructure/code and custom DSL to solve this issue.
There's also problems it causes:

• html macro is different than all our other response methods (json, plain_text, etc.) and you can't set an http status
• We haven't extended this DSL to our components, do we re-implement the expose code in pages to allow passing data down to components?
• Compilation error messages are UGLY. Good luck trying to figure out what exactly you forgot to pass to your page when theirs 10 or more arguments in the initialize method.

All this makes me want to rethink how we do this for pages. The question I have mulled over for quite a while is, "Can we accomplish something that solves 70-90% of the problem without a DSL?"

I was commenting on an issue and said that someone could probably just monkey-patch their state that they need everywhere onto HTTP::Server::Context since it seems like that's what it's for and it hit me. The context object is absolutely where state we want passed everywhere could go. To be clear maybe not HTTP::Server::Context exactly but why not a MyApp::CustomHTTPContextWrapper that wraps the http context and adds other accessors for things like current_user. It doesn't even have to be used in the action level, it could just be made to pass into a page and there could be a method used so that you can hide all the data that's getting wrapped up.

class Secrets::Show < BrowserAction
  get "/secrets/:secret_id" do
    html ShowPage.new(context: authenticated_context, secret: SecretQuery.find(secret_id))
  end

  def authenticated_context
    AuthenticatedContext.new(wrapped: context, current_user: current_user)
  end
end

class Secrets::ShowPage < MainLayout
  getter context : AuthenticatedContext
  getter secret : Secret

  def initialize(@context, @secret)
    # context.request
    # context.current_user
  end

  # ...
end

class AuthenticatedContext
  getter current_user : User
  getter wrapped : HTTP::Server::Context
  delegate request, response, to: wrapped

  def initialize(@wrapped, @current_user)
  end
end

With this implementation we almost get the same level of simplicity with none of the macro DSLs. Plus, if you don't use anything that needs the context, you don't have to pass it in. The url helpers require it, but if you never use the helpers, it will never complain about the context missing since it accesses the context like @context and without specifying the type.

[stephendolan]

Hmm... the introduction of the need for an initialize method in every Page seems less than ideal, but the +114, -583 diff is pretty enticing.

I also wonder what this would look like for someone like me, where I just want my context available in every page/component without repeating the getter for every single file. Do I just add the getter to MainLayout and then need to remember to call super for every page, then do the same thing for my components?

I kind of have the same concern with having to pass context: authenticated_context to every page render in the application, but maybe there's a nice global way to tackle that as well that's more accessible now that we're back in plain-Crystal world?

[matthewmcgarvey]

To me, the price of typing 20 characters is pretty low compared to the confusion of why we have a magic DSL in Lucky

[stephendolan]

Have we had lots of folks that have been confused by needs, or is the confusion/trouble more on the framework maintenance front? If the latter, I don't think it's fair to do a price comparison so plainly when you're shifting the price to an entirely different group of people.

If it was 20 characters in the app as a whole I'd agree, but if you have 200 pages that need the context, you're repeating yourself for all 200 pages without a clear way to DRY up the code.

I also think Rails is evidence of folks not minding (somewhat confusing) magic if it means they end up with less boilerplate/thought around stuff that they'd expect the framework to pass around for you.

[robacarp]

I think I actually prefer the needs pattern myself, though the compiler error messages are indeed awful when you get it wrong. A DSL isn't in an of itself a bad thing, Crystal itself is a DSL on top of llvm and it's great.

I have two layouts (Page base classes) in my app: one for logged in users and one for not-logged-in users.
I have two Action base classes as well: one for logged in users and one for not-logged-in users.

Being able to simply put html ShowPage, current_object: object is nice, and it makes the developer not have to think that "oh right, I need to pass in the current user because this is a logged in view" instead the other dev three teams over can just write the layout and needs the user and it gets passed in every time automatically.

As an application grows in complexity, this kind of encapsulation is necessary. The job of an action isn't to know the entire application it lives in and be able to re-create everything needed for it, it's simply to build the objects that it's page cares about. That's also why frameworks inevitably end up adding an instrumentation framework where events can be listened to and acted upon without sprinkling code in every page / query / action.

People get really frustrated by DSLs because it can be difficult to track down where the code actually gets run... but an eloquent DSL is a powerful way to express solutions to a problem and reduces cognitive burden -- that's what a programming language is in the first place. Ruby has many fantastic examples of quality DSLs and Lucky does too.

[jwoertink]

The intention behind "needs" was to allow for passing data down into html pages without having to do it in every place we render each page.

This is not quite correct. We added needs as a way to make your pages type-safe by ensuring you don't forget to pass over data that's required. We kept the DSL similar across the whole Lucky framework which is why needs exists in several different locations. This type-safety mechanism is pretty much the core of why Lucky exists.

Overall, I'm torn on this concept... I can see some things would be really good, but also some I'm not sure on.

Can we accomplish something that solves 70-90% of the problem without a DSL?

I've never really been a huge fan of all the DSL stuff, but I also get it. For me, DSL should be salt & pepper on top. Just small sprinklings to enhance where needed.

I do think it would be nice to be a bit more of just "Raw Crystal". It makes it easier when someone says "How do I do X?", and we say "It's all just Crystal.. make a class, write a method, and call it!".

AuthenticatedContext.new(wrapped: context, current_user: current_user)

I love this idea. I'm all for the pattern of passing a single context object instead of multiple arguments. Also not attaching directly to HTTP::Server::Context.

class Secrets::ShowPage < MainLayout
  getter context : AuthenticatedContext
  getter secret : Secret

  def initialize(@context, @secret)
    # context.request
    # context.current_user
  end
end

This setup worries me though. Aside from the extra typing across tons of pages, I feel like it could become problematic from a type-safe standpoint. Being able to do this should be looked at more as an escape hatch rather than the recommended path.

Maybe we can just look at creating a base abstract Context class. Lucky::PageContext or something.. Then if we can figure out how to make it always know about the HTTP::Server::Context, and you can add your additional bits after the fact. Then we just guide people to passing that in to their components as needed? 🤔

[matthewmcgarvey]

To be clear, this is no less type safe than the needs functionality. In fact, I would say it's more type safe as lucky pages currently ignore any arguments that they are passed and don't need. Remove a dependency for a page that requires 3 sql queries to retrieve? You'll have to hunt down all the places it was passed in because lucky will just ignore it.

I think I should point out, I don't like the needs stuff. I think it overwrites basic Crystal coding (initialize blocks and properties) and doesn't have a good way to undo it or go without it.

You'll find if you look at any other web framework that is statically typed, this is the kind of solution they go with.

• Go - Buffalo
• Rust - Rocket
• Java - Spring

This is also not to say that we can't have the needs stuff, I guess. The only thing I would want, though, is that it's not in the HTMLPage module and instead added in the application so that it can be taken out

[robacarp]

@matthewmcgarvey when talking about DSLs and crystal primitives, I wonder if you're drawing a distinction I'm not seeing between needs and other macro space DSLs. How does needs differ from before in an action, or table in a model?

[matthewmcgarvey]

@robacarp I'm not against DSLs, but if a DSL is restrictive and missing functionality it makes sense that there should be a way to go around it.

In the case of needs there's not a straightforward way to get around it. As a maintainer and knowing the code, I know exactly which methods I'd need to call to specify my own initialize function in a page but I'd still have to satisfy the underlying needs code if I wanted to go down that route. That's restrictive.

Then there's issues and missing functionality. The most obvious one we've already mentioned above is that components don't have the same infrastructure code that pages do. But there's others, like you can't expose a method with a question mark even though the docs say it passes the result of a method call down into pages, and compilation errors are confusing with initialize methods because the user didn't make the method.

So I'm wondering, is the DSL covering up a genuinely complicated or verbose alternative? That's what I'm attempting to experiment with here. This is just one idea of how to solve it without a DSL. I think there's more things to consider with this context idea but I'm focusing on the needs thing right now.

One thing I'd like to come out of this little experiment is that there is not an "escape hatch" from the needs stuff right now.

[matthewmcgarvey]

O ya, the other point:

• before pipes wrap a sufficiently complex concept and are entirely avoidable (all the code could be done in the routes)
• table is another place I'd like to see changed. It's wrapping VERY complex code, but there is no other way around it though that is not at all why I'd want it changed - one experiment was done here - another drastic experiment was done here

[jwoertink]

I've been thinking about this. I may be down for going this route, but I don't see this as necessary to get us to 1.0. The original issue we were trying to solve was passing a ton of things in to components. Nothing is actually broken or missing, it's just more of "Can we make this better?".

Right now I'd like to focus on changes that are actually broken / missing that can get us to 1.0 and revisit this after.

[paulcsmith]

I’m pretty in favor of keeping needs. I’m not a fan of writing the initialized and making different contexts for things. I don’t have a ton of time to go in depth but my main reasons are that:

• I think it looks nicer. Subjective but I do find it easier to read and write
• I don’t love having to pass the same context to every page, and declare the same context in the initializer. That doesn’t seem very DRY and I think would become a bit of a hassle.
• I also feel that it makes it harder to focus on the core parts of the page when reading it because you have more duplicated code between pages

However, the error messages are a pain. That’s for sure. Maybe there is a way to improve those in the macro?

I do think ignoring unused variables is not as safe, but I think it is far more convenient and user friendly. We didn’t do that at first and people had a REALLY hard time with it. In an ideal world id love to not do it but it seems most applications have enough edge cases in their logic that the trade off seems worth it. I do think we should probably remove allowing unused arguments from components though. I think I’m those cases it can be a pain

[robacarp]

@matthewmcgarvey thanks for the explanation. I thought that might be where you were headed with that, and I agree with you in that principle.

What I like to see is the best of both worlds. I want a low-barrier to entry of boilerplate code so I can tear through the busywork of standing up an application CRUD as fast as possible. In any app there are 6 magic endpoints which do the "important" work and there are 350 endpoints that just need to be the same copy-pasta CRUD. In the in the rare case where it's insufficient I'd like to be able to override it, but in the landslide majority case I want it to be terse, readable, and easy to learn.

Maybe needs as it is currently implemented doesn't serve that last case -- but as yet I haven't found a (ahem!) need that it doesn't suffice.