search

ViewComponent / view_component

A framework for building reusable, testable & encapsulated view components in Ruby on Rails.
https://viewcomponent.org
MIT License
3.17k stars 404 forks source link

Add strict helpers #1987

Open reeganviljoen opened 3 months ago

reeganviljoen commented 3 months ago

closses #1976

What are you trying to accomplish?

Add a config.view_component.strict_helpers_enabled mode to view component which will throw an ViewComponent::StrictHelperError when helpers.<some_helper> is used.

This

What approach did you choose and why?

Anything you want to highlight for special attention from reviewers?

joelhawksley commented 2 months ago

@reeganviljoen thoughts on my comment from my review above?

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?

reeganviljoen commented 2 months ago

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?

@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

boardfish commented 2 months ago

If a gem providing ViewComponents uses helpers, I'm not sure we should mandate that it's subject to strict_helpers in the same way as the rest of the app.

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:

  1. Gems adhere to their own configuration. I don't think there's a layer of config in place to do this just yet – gems still inherit from app config.
  2. Enabling strict_helpers enforces this for gems too. Making this available as an option would be good, but I don't think it should be the only one.
  3. We expose a lambda for strict_helpers. This could allow folks the flexibility to allowlist on a gem-by-gem basis, or report strict helper violations (e.g. to an error reporting platform/in logs). I'd like to see this become something we do more often potentially. If we introduce configuration that controls a breaking change, this would be good to allow folks to understand (through their test suite or at runtime) where changes need to be made.

I'm sure there are options beyond that, but that's what comes to mind right now. Any thoughts?

joelhawksley commented 2 months ago

@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.

reeganviljoen commented 2 months ago

@joelhawksley @boardfish noted, I wills see if I can work on something to macro based system working