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

Include Hanami::Action::Response#exposures in view rendering context #319

Merged
merged 3 commits into from Nov 14, 2021

Conversation

jodosha
Copy link
Member

@jodosha jodosha commented Jul 2, 2020

Syntax preview

class MyAction < Hanami::Action
  def handle(*, res)
    res[:users] = repo.all
    res[:foo] = "X"
    res.render(view, foo: "Y")
  end
end

The :users exposure is added to the view args for rendering.

In case of overlapping names between inline options passed to Hanami::Action::Response#render and Hanami::Action::Response#exposures, inline options will take the precedence.

In the example above the view will receive "Y" as value for :foo.

@jodosha jodosha added this to the v2.0.0 milestone Jul 2, 2020
@jodosha jodosha self-assigned this Jul 2, 2020
@jodosha jodosha requested a review from timriley July 2, 2020 13:46
@timriley
Copy link
Member

timriley commented Jul 6, 2020

@jodosha nice addition! I've left a comment with a suggestion that I think should improve the clarity of handling clashing exposures/view render options.

If you agree on that, it'd be nice to get this in.

Longer term, I think we want to have a good think/discussion about the approach we recommend for application authors to follow re: passing objects to views and make sure that's well reflected in our future documentation. With both response exposures and view exposures existing, I can see things getting pretty confusing.

@jodosha jodosha requested a review from timriley January 7, 2021 09:22
@jodosha jodosha changed the base branch from unstable to main June 15, 2021 08:22
@cllns
Copy link
Member

cllns commented Nov 8, 2021

After merging #345, I realized this PR might provide the same functionality. It does it in a different place and higher up the call stack.

Which way is better @jodosha? Happy to use this one and revert #345. My understanding is that this PR would handle cases of people using hanami-controller with hanami-action without using ApplicationAction (which is what's used for hanami/hanami.) and #345 would not handle that case.

@jodosha
Copy link
Member Author

jodosha commented Nov 8, 2021

@cllns @timriley Two things. ✌️ Please let me know if you agree.
@cllns In case of agreement, would you be able to expand this PR?

One: I would say it's better to push down to generic Action, even outside of hanami.


Two: We should consider the following usages:

Response exposures

We set an exposure in the response, which should be available to the corresponding view/template. This is what #345 is taking care of.

class MyAction < Hanami::Action
  def handle(*, res)
    res[:foo] = "bar"
  end
end

Rendering exposures

We explicitly set an exposure while invoking res.render. :foo must be available in the view/template. This is what this PR is taking care of. Dunno if this PR also covers #345 as well.

class MyAction < Hanami::Action
  def handle(*, res)
    res.render(view, foo: "bar")
  end
end

A mix of the previous use cases

Both :color and :foo must be available in the view/template. In case of conflict, the Hanami::Reponse#render wins.

class MyAction < Hanami::Action
  def handle(*, res)
    res[:color] = "red"
    res.render(view, foo: "bar")
  end
end

@timriley
Copy link
Member

timriley commented Nov 9, 2021

@jodosha I agree with both your suggestions, and would love to see this PR expanded to cover them! @cllns, let me know what you think and if you'd be happy to work on this 😄

@cllns
Copy link
Member

cllns commented Nov 9, 2021

@timriley @jodosha Yup, I can see this PR through

jodosha and others added 3 commits November 12, 2021 10:55
We don't need to send the exposures twice.

We first implemented this in #345 but Hanami::Action::Response.render
handles sending the exposures to the view now (and ApplicationAction
uses that)
@cllns cllns force-pushed the enhancement/view-context-exposures branch from 6c53dd9 to e1918d8 Compare November 12, 2021 16:08
@cllns
Copy link
Member

cllns commented Nov 12, 2021

Rebased this branch, removed the change from #345 and all the tests still pass 🙌 . So this original change to Hanami::Action::Response#render does handle the actual sending the exposures to the view and we don't need to do it twice. Now it's at the appropriate spot, since it'll work for both StandaloneActions and ApplicationActions.

@jodosha @timriley please review

@cllns cllns self-assigned this Nov 12, 2021
@jodosha
Copy link
Member Author

jodosha commented Nov 13, 2021

@cllns LGTM 👍 I'm the original author of the PR, so I can't approve it 😉

Copy link
Member

@timriley timriley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great 👍🏼

@cllns cllns merged commit 12d1ee3 into main Nov 14, 2021
@cllns cllns deleted the enhancement/view-context-exposures branch November 14, 2021 23:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants