Add strict helpers #1987

reeganviljoen · 2024-02-12T07:23:31Z

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

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 · 2024-02-14T21:31:28Z

lib/view_component/base.rb

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


I wonder if this feature might be less confusing if we just had it be config.helpers_enabled and defaulting it to true?

@joelhawksley I think that's a good idea, when I have some time I will apply this feedback

@joelhawksley I have changed the config to this, if you could review it again it would be a massive

joelhawksley · 2024-03-04T22:35:57Z

@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 · 2024-03-06T06:01:34Z

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 · 2024-03-07T07:59:48Z

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:

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.
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.
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 · 2024-03-07T19:52:32Z

@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 · 2024-03-14T13:40:00Z

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

danini-the-panini · 2024-07-04T07:47:39Z

I think we might be better off having this be a macro folks can set on a component, likely on their ApplicationComponent.

I am on board with this ☝🏻

Could we also allow it to be optionally a deprecation warning instead of an error, that way it won't break things but it will give devs the tool to migrate their existing components. This could be useful for e.g. very large code bases where it is infeasible to migrate all components at once

danini-the-panini · 2024-07-04T08:43:30Z

Another useful feature might be allowing to pass in a block that gets called every time helpers is referenced, so that devs can hook into their error tracker e.g. Sentry

joelhawksley · 2024-08-02T15:16:17Z

@reeganviljoen just checking in- are you planning to complete this work?

reeganviljoen · 2024-08-02T15:34:31Z

@joelhawksley Absolutely, just been verry busy

reeganviljoen · 2024-08-07T19:31:42Z

@joelhawksley what do you think about @danini-the-panini comment, I think its a good idea, I could work that into our current instrumentation setup in a seperate PR

reeganviljoen · 2024-08-07T19:34:33Z

@joelhawksley I think that this api is complete as the main concern was gem compatibility but the recent addition to use_helpers provides a path to working. with gems in a strict mode

joelhawksley · 2024-08-08T17:26:14Z

@reeganviljoen thanks for cleaning up this PR. For now, I'm going to mark it as blocked on @boardfish's work here: #1945 (comment) ❤️

boardfish · 2024-09-04T15:54:08Z

This might actually be ideal as the first component-local config option due to the small scope of the PR. I'll see if I can grab some time this weekend to figure out if I can use this to make a start.

boardfish · 2024-09-08T13:34:58Z

Had a very scrappy crack at this in 35017a9 – it looks possible, and we could probably even ship that with a few more tests behind it and some cleanup (still need to document things and get the use of the private variable names vs. public method names straight). Going forward, though, we probably want to make a nice internal DSL that'll support migrating these config options across slowly but surely.

reeganviljoen · 2024-09-08T17:36:14Z

@boardfish I aprreciate the help, if you want to pair on this issue I be happy to help.

boardfish · 2024-09-08T18:40:08Z

There's perhaps an even cleaner solution in ActiveSupport::Configurable that I've now found - we were previously including this but hadn't perhaps used it to its full extent as far as I'm aware. I've got a busy week ahead but I'd like to look at wrapping it up as soon as I can to unblock you - I'd appreciate any early feedback anyone has on the PR.

reeganviljoen · 2024-10-08T18:49:40Z

@boardfish I have rebased of of #2124 and made that the base branch for this to make sure there is no weird diff issues

boardfish · 2024-10-09T08:16:52Z

Awesome ✨ In #2092, the changes to Base#inherited introduce component-local config – if you add those to this PR, where you're currently checking the helpers_ config options against ViewComponent::Base, you should instead be able to use component_config on the component instance. You'll also need to update some of the tests so that they don't stub the global config and instead use the value from the component's local config.

boardfish · 2024-10-11T08:13:07Z

On reflection, I'm thinking that the implementation I've got here (to be included in this branch) probably might need to change a touch – that makes all global options available on component-local config, when in reality changing any settings other than the ones related to helpers won't do anything. I expect that might cause some confusion if left in.

Perhaps it would be good to have a component_defaults key in the global config which things that inherit from ViewComponent::Base will use an inheritable_copy of instead.

* Fix duplicate template error bug This change fixes the bug reported where a component will report that it has both a template and a `call` method defined despite only having an HTML template. I was able to reproduce this bug by pulling in Avo (like reported in the initial bug report) and running `vegeta` (to trigger parallel requests since I had a hunch this was a race condition): ``` echo "GET http://localhost:3000/avo/resources/cars/1" | vegeta attack -duration=2s ``` This reliably reproduced the error: ``` ActionView::Template::Error (Template file and inline render method found for Avo::CoverPhotoComponent. There can only be a template file or inline render method per component. Template file and inline render method found for variant '' in Avo::CoverPhotoComponent. There can only be a template file or inline render method per variant. Templates: ``` I added _a lot_ of debug logs to understand the race condition that I thought was occurring, and realized that multiple threads are calling `gather_templates` _and_ mutating the `@templates` array at the same.When looking at the old compiler code and realized that this likely isn't new behavior. This led the investigation towards how we collect and surface errors or otherwise might modify templates. It turns out there's a difference in the new and old compiler code after the refactor: ```ruby \# old def template_errors @__vc_template_errors ||= \# new def gather_template_errors(raise_errors) errors = [] ``` _We're not memoizing the errors like we used to_. This is more correct behavior, but explains how a race condition would make this error case much more difficult to occur in older versions of the compiler. This change brings us back to the old behavior by memoizing the errors we collect in `gather_template_errors` but the `@templates` ivar is still being mutated. I don't want to change _too much_ in this PR, but a subsequent change might be wrapping the entire `compile` method in the `redefinition_lock` (and renaming it to `compile_lock`) to avoid similar issues in the future. I did not include a test because it's dfficult to reproduce this race condition reliably in the test environment. I believe this _should_ close out ViewComponent#2114 * Fix tests, raise errors for compiled components with errors * There isn't always a default template when there are errors. * Make standard happy ugh * Remove debug code * Fail compilation when errors present * Revert default template change * Add informative comment * Add changelog --------- Co-authored-by: Joel Hawksley <[email protected]>

boardfish · 2024-10-11T08:46:23Z

Made those changes myself, but there are a few icky commits left in – I figure that if the base PR is merged, those will be easier to clean up.

reeganviljoen · 2024-10-11T09:20:47Z

@boardfish much thanks will take a look tonight

reeganviljoen requested a review from boardfish as a code owner February 12, 2024 07:23

reeganviljoen requested review from camertron, joelhawksley, boardfish and BlakeWilliams and removed request for boardfish February 12, 2024 07:23

reeganviljoen force-pushed the rv-add-strict-helpers branch from d6fe052 to 3c74fd8 Compare February 12, 2024 07:27

joelhawksley reviewed Feb 14, 2024

View reviewed changes

reeganviljoen requested a review from joelhawksley March 4, 2024 06:18

joelhawksley mentioned this pull request Mar 29, 2024

Rails Engine support #1945

Closed

reeganviljoen mentioned this pull request May 27, 2024

Add from to use_helpers to add macro like syntax #2034

Merged

joelhawksley added the blocked label Aug 8, 2024

reeganviljoen mentioned this pull request Aug 9, 2024

Add helper instrumentation #2072

Closed

boardfish mentioned this pull request Sep 8, 2024

WIP: Extract strict_helpers_enabled? to component-local config #2092

Draft

reeganviljoen added 9 commits October 8, 2024 20:45

add: strict helpers

ab59bdd

Merge branch 'main' into rv-add-strict-helpers

0ada155

revert "fix: linting errors"

6508f28

fix tests

b292de5

replace helper delegation with use_helpers

380fe22

replace strict_helpers_enabled with helpers_enabled

fca14b1

add: test for missing coverage

cb73965

fix: comment location for csrf and csp nonces

6d4bed1

update changelog

a4a7ed9

reeganviljoen force-pushed the rv-add-strict-helpers branch from 313a922 to a4a7ed9 Compare October 8, 2024 18:48

reeganviljoen changed the base branch from main to introduce-component-local-config October 8, 2024 18:50

boardfish force-pushed the introduce-component-local-config branch from 161cee3 to 6378768 Compare October 10, 2024 11:18

BlakeWilliams and others added 5 commits October 11, 2024 09:27

release 3.17.0 (ViewComponent#2125)

93534b1

Update CHANGELOG.md

0470809

Fix preview_paths in docs (ViewComponent#2127)

b7a6ad0

Derive strict_helpers_enabled from component-local config

dbfc516

boardfish force-pushed the rv-add-strict-helpers branch 3 times, most recently from 70bed0a to dbfc516 Compare October 11, 2024 08:44

boardfish force-pushed the introduce-component-local-config branch 2 times, most recently from e9592cb to 37e6d0d Compare October 20, 2024 09:37

boardfish mentioned this pull request Oct 20, 2024

Make accommodations for component-local config #2124

Open

boardfish force-pushed the introduce-component-local-config branch 2 times, most recently from 2dcc88d to 62e0960 Compare October 21, 2024 10:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add strict helpers #1987

Add strict helpers #1987

reeganviljoen commented Feb 12, 2024

joelhawksley left a comment

joelhawksley Feb 14, 2024

reeganviljoen Feb 16, 2024

reeganviljoen Mar 4, 2024

joelhawksley commented Mar 4, 2024

reeganviljoen commented Mar 6, 2024 •

edited

Loading

boardfish commented Mar 7, 2024 •

edited

Loading

joelhawksley commented Mar 7, 2024

reeganviljoen commented Mar 14, 2024 •

edited

Loading

danini-the-panini commented Jul 4, 2024 •

edited

Loading

danini-the-panini commented Jul 4, 2024

joelhawksley commented Aug 2, 2024

reeganviljoen commented Aug 2, 2024

reeganviljoen commented Aug 7, 2024

reeganviljoen commented Aug 7, 2024 •

edited

Loading

joelhawksley commented Aug 8, 2024

boardfish commented Sep 4, 2024

boardfish commented Sep 8, 2024

reeganviljoen commented Sep 8, 2024

boardfish commented Sep 8, 2024

reeganviljoen commented Oct 8, 2024 •

edited

Loading

boardfish commented Oct 9, 2024 •

edited

Loading

boardfish commented Oct 11, 2024 •

edited

Loading

boardfish commented Oct 11, 2024

reeganviljoen commented Oct 11, 2024

Add strict helpers #1987

Are you sure you want to change the base?

Add strict helpers #1987

Conversation

reeganviljoen commented Feb 12, 2024

What are you trying to accomplish?

What approach did you choose and why?

Anything you want to highlight for special attention from reviewers?

joelhawksley left a comment

Choose a reason for hiding this comment

joelhawksley Feb 14, 2024

Choose a reason for hiding this comment

reeganviljoen Feb 16, 2024

Choose a reason for hiding this comment

reeganviljoen Mar 4, 2024

Choose a reason for hiding this comment

joelhawksley commented Mar 4, 2024

reeganviljoen commented Mar 6, 2024 • edited Loading

boardfish commented Mar 7, 2024 • edited Loading

joelhawksley commented Mar 7, 2024

reeganviljoen commented Mar 14, 2024 • edited Loading

danini-the-panini commented Jul 4, 2024 • edited Loading

danini-the-panini commented Jul 4, 2024

joelhawksley commented Aug 2, 2024

reeganviljoen commented Aug 2, 2024

reeganviljoen commented Aug 7, 2024

reeganviljoen commented Aug 7, 2024 • edited Loading

joelhawksley commented Aug 8, 2024

boardfish commented Sep 4, 2024

boardfish commented Sep 8, 2024

reeganviljoen commented Sep 8, 2024

boardfish commented Sep 8, 2024

reeganviljoen commented Oct 8, 2024 • edited Loading

boardfish commented Oct 9, 2024 • edited Loading

boardfish commented Oct 11, 2024 • edited Loading

boardfish commented Oct 11, 2024

reeganviljoen commented Oct 11, 2024

reeganviljoen commented Mar 6, 2024 •

edited

Loading

boardfish commented Mar 7, 2024 •

edited

Loading

reeganviljoen commented Mar 14, 2024 •

edited

Loading

danini-the-panini commented Jul 4, 2024 •

edited

Loading

reeganviljoen commented Aug 7, 2024 •

edited

Loading

reeganviljoen commented Oct 8, 2024 •

edited

Loading

boardfish commented Oct 9, 2024 •

edited

Loading

boardfish commented Oct 11, 2024 •

edited

Loading