-
Notifications
You must be signed in to change notification settings - Fork 404
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
Add strict helpers #1987
base: main
Are you sure you want to change the base?
Add strict helpers #1987
Conversation
d6fe052
to
3c74fd8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thought on naming.
As a bigger question: what happens if one turns this on for their app and then wants to use a gem that provides ViewComponents that use helpers?
lib/view_component/base.rb
Outdated
@@ -224,7 +232,7 @@ def controller | |||
# @return [ActionView::Base] | |||
def helpers | |||
raise HelpersCalledBeforeRenderError if view_context.nil? | |||
|
|||
raise StrictHelperError if ViewComponent::Base.config.strict_helpers_enabled |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this feature might be less confusing if we just had it be config.helpers_enabled
and defaulting it to true?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joelhawksley I think that's a good idea, when I have some time I will apply this feedback
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joelhawksley I have changed the config to this, if you could review it again it would be a massive
@reeganviljoen thoughts on my comment from my review above?
|
@joelhawksley I don't think this will break any existing gems, unless the user enables the config, however if the user does, it might not work unless the gem documents which helpers are used so the user can add them themselves with the use_helpers method I do think this is a positive improvement however, as just adding helpers increases coupling to global state, which I feel is bad and is against the whole point of view_component. If a gem wants to add helpers they should let the user decide what helpers and how much coupling to global state they want |
If a gem providing ViewComponents uses helpers, I'm not sure we should mandate that it's subject to Gems providing components that use helpers would need to be updated to use the new syntax, and waiting on providers to do that doesn't feel like an ideal experience for devs using ViewComponent to me. It's absolutely something gem maintainers should do post-haste, but I think configuration could make the transition period easier for consumers of these gems. I can see us going a few different ways with this:
I'm sure there are options beyond that, but that's what comes to mind right now. Any thoughts? |
@boardfish thanks for sharing your concerns- I generally agree with you. I think we might be better off having this be a macro folks can set on a component, likely on their ApplicationComponent. |
@joelhawksley @boardfish noted, I wills see if I can work on something to macro based system working |
closses #1976
What are you trying to accomplish?
Add a
config.view_component.strict_helpers_enabled
mode to view component which will throw anViewComponent::StrictHelperError
whenhelpers.<some_helper>
is used.This
What approach did you choose and why?
Anything you want to highlight for special attention from reviewers?