amberframework / amber

A Crystal web framework that makes building applications fast, simple, and enjoyable. Get started with quick prototyping, less bugs, and blazing fast performance.
https://amberframework.org
MIT License
2.57k stars 206 forks source link

Added rails-like locals functionality for partials #1220

Closed AndiLavera closed 4 years ago

AndiLavera commented 4 years ago

Description of the Change

Added a simple way to pass locals to partials. I know all variables are accessible in partials. Sometimes you want to rename those variables so the context makes more sense in said partial.

Usage in a view would be:

render(partial: "people.ecr", locals: {name: user.name, email: user.email, age: user.age})

# In people.ecr
<h1>Hello, <%= name %><h1/>

Alternate Designs

Currently the alternative is using the variables by the name they were assigned in the controller.

Benefits

Allows users to rename variables without actually having to initialize them in a controller before rendering a partial.

Possible Drawbacks

None that i am aware of. I didn't write any specs for it though. I could do so if requested!

robacarp commented 4 years ago

The last time I looked, templates (both partials and otherwise) render in the implicit scope of the method which calls render. This is because render is a macro, so this code and the template code itself (also a macro) is functionally inlined at compile time. In Rails, a subset of locally available named variables are sent to the template context, which is constructed manually and carefully.

The paradigm difference makes me ask why things are so segregated in Rails, and if the implicit scope leakage could cause problems. For example, what if I have a template which needs a name variable and a partial which needs one of the same name? When render(partial:, locals: {name:}) is called will it overwrite the variable above?

My instinct as a view-writing developer have been thrown off by this more than once because I assume that the scope of a partial is separate from the scope of a parent template as the scope of a composed method is separate. If that is not the case, I think this change is risky.

AndiLavera commented 4 years ago

The last time I looked, templates (both partials and otherwise) render in the implicit scope of the method which calls render. This is because render is a macro, so this code and the template code itself (also a macro) is functionally inlined at compile time.

This is correct.

For example, what if I have a template which needs a name variable and a partial which needs one of the same name? When render(partial:, locals: {name:}) is called will it overwrite the variable above?

Yes, it definitely would overwrite the first name variable.

I think this change is risky.

I didn't think about overwriting variables as I have never run into this issue. However, I agree. I believe we can close this.

I assume that the scope of a partial is separate from the scope of a parent template.

To be honest, I was quite surprised to find out that all local variables are in scope & original thought it to be a bug. But if we, & probably others, assume partials create a new scope, why doesn't it? I understand how the render macro works. I am specifically asking why this isn't something we as a community discuss, design & code.

My goal of this PR wasn't "too pass variables to partials" as they are already in scope. It was for allowing a more explicit statement. Maybe the render method/macro needs to change before something like this PR can be merged?

robacarp commented 4 years ago

My goal of this PR wasn't "too pass variables to partials" as they are already in scope. It was for allowing a more explicit statement. Maybe the render method/macro needs to change before something like this PR can be merged?

Yeah of course. I think the behavior you expect is the right one, it's just not the one that's currently in there in all the ways you expect -- if you write the code as if the scope has to be @global variables, and then reference those as such, it works but not for the reasons you think it works. Which is probably more dangerous.

I'd love to switch the paradigm to something where the render macro is responsible for building an explicit scope which is wrapped around the template render, but as far as I know that's not a construct that Crystal provides (yet).

AndiLavera commented 4 years ago

@robacarp Messaged you on gitter to continue this. This PR can be closed. Knowing user's may overwrite their own variables without knowing is dangerous.