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

Make helper macros public #169

Merged
merged 2 commits into from
Mar 22, 2024
Merged

Make helper macros public #169

merged 2 commits into from
Mar 22, 2024

Conversation

joeldrapper
Copy link
Collaborator

@joeldrapper joeldrapper commented Mar 18, 2024

This PR consolidates the with and without capture block helper macros and makes them part of the public API available in Phlex::SGML and Phlex::CSV classes as register_value_helper and register_output_helper.

Closes #168

@joeldrapper joeldrapper added this to the 1.2 milestone Mar 18, 2024
@bradgessler
Copy link
Contributor

Picking up the naming convo here: is the difference between HTML and value helpers one of safely escaping HTML? If so I'm thinking the method names should suggest that like register_html_safe_helper and register_html_unsafe_helper

Another possibility, if those are unclear, is two methods that could be chained like this:

register_helper :pluralize
register_helper html_safe :local_time

To be most consistent with Rails helpers class method on controllers that exposes controller methods to views, change register_helper to helper and offer a helpers method that that accepts a list of symbols to expose.

@willcosgrove
Copy link
Collaborator

willcosgrove commented Mar 18, 2024

is the difference between HTML and value helpers one of safely escaping HTML?

I don't think it has anything to do with escaping HTML output. It's about the intent behind the helper. When you call pluralize(...) you intend to get back a string as a return value. When you call link_to(...) you intend for an <a> tag to be added to the buffer.

register_value_helper is for when you want the return value (pluralize, class_names, etc)
register_output_helper is for when you want the output buffer modified (link_to, csrf_meta_tags, etc)

@joeldrapper
Copy link
Collaborator Author

Right, you’ll have HTML safety in either case.

As @willcosgrove mentioned output helpers like link_to push to the buffer and return nil, whereas value helpers like pluralize, asset_url, etc. push nothing to the buffer and return a value for you to use as content or attributes of some other tag.

@bradgessler
Copy link
Contributor

bradgessler commented Mar 19, 2024

Got it, those are good explanations, thanks!

I think in Rails a helper behaves similarly to register_value_helper in that it doesn't modify the buffer, so to maintain party register_value_helper could be named helper and helpers.

Not sure what to name an output_helper in this scenario though 🤔

@joeldrapper
Copy link
Collaborator Author

I’d quite like to deprecate helpers so it’s probably better to have parity with register_element.

@willcosgrove
Copy link
Collaborator

Why do you want to deprecate helpers? It seems like a reasonable escape hatch. If helpers is gone I would guess that means there's no way to access the view_context with a public API right?

@bradgessler
Copy link
Contributor

I think my last message confused helpers

What I mean by helper and helpers is to achieve parity with the class methods at https://api.rubyonrails.org/classes/ActionController/Helpers.html within Phlex, which would be accessible via class methods (not the current instance method)

@joeldrapper
Copy link
Collaborator Author

I guess it is a reasonable escape hatch. I’d at least like to burry it deep in the documentation. It’s the thing that seems to cause the most confusion for people using Phlex with Rails and I think it’ll be much simpler to point folks to these macros.

@bradgessler
Copy link
Contributor

I think it's reasonable to get rid of helpers.blah in the context Joel is referring to if the register_*_helper API is made public and is obvious. I agree with Joel that its more confusing having two ways of accessing helpers over one.

What I think might be confusing for people is to determine the difference between a value vs output helper, which is still a problem with the current helpers API. Is there anything that can be done for that with better error messages or something?

@willcosgrove
Copy link
Collaborator

I think it's reasonable to get rid of helpers.blah [...] its more confusing having two ways of accessing helpers over one.

I agree, and I would be in favor of a rename of the helpers method as it currently is to something like view_context. And I don't think it needs to be front and center in the docs, since it will likely not be used much. But I think it's important to keep it available.

@bradgessler
Copy link
Contributor

I think this is my last thoughts on naming after thinking about it for a few days and looking at some of the names in Rails source code.

The register_output_helper seems congruent to names I see in Rails source like OutputSafetyHelper, OutputBuffer, etc. I think this could also be called register_buffer_helper, but I don't feel strongly either way.

It's the register_value_helper that's been bugging me more—I'd propose that register_helper should be considered if there's no plans to try to automatically detect the difference between "output" and "value" helpers. The rational is that there's nothing "special" about these helpers in Ruby or Rails—they're just functions in a module that return a string. Calling them out as "value" makes me pause and ask myself, "is there something special about these helpers that I need to think about?"

All things considered, if the PR was committed as-is, I think the names are reasonable.

@joeldrapper
Copy link
Collaborator Author

There’s no way I can think of that we could use to detect the difference. I get what you mean about have a “default” helper as just register_helper, but I’d quite like the naming to make you think about what you’re using this for as it‘s not immediately obvious why you need to choose between them. Hopefully we can make that nice and clear in the documentation.

@joeldrapper joeldrapper merged commit fab2934 into main Mar 22, 2024
13 checks passed
@joeldrapper joeldrapper deleted the helper-macros branch March 22, 2024 22:40
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

Successfully merging this pull request may close these issues.

Expose output helpers to ApplicationComponent
3 participants