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

Reorganise configuration across Rails app and component classes #2210

Draft
wants to merge 14 commits into
base: v4
Choose a base branch
from

Conversation

boardfish
Copy link
Collaborator

@boardfish boardfish commented Feb 15, 2025

This PR aims to rework configuration such that individual component instances can be configured differently, with the end goal of ensuring that applications and engines can define their configuration separately. We're taking a scorched earth approach here, though it will be important to ensure that the migration path from v3 is straightforward.

NOTE: This branch is a WIP – I'll squish things down before it's ready to share.

#1945 sheds some good light on the kinds of problems we're looking to solve here. Big thanks to @Slotos for their really helpful comment on that issue.

@boardfish boardfish force-pushed the configuration-rework branch from c7cf9a7 to f6afae9 Compare February 16, 2025 11:46
@boardfish boardfish force-pushed the configuration-rework branch from f6afae9 to b159d1e Compare February 19, 2025 09:09
@boardfish
Copy link
Collaborator Author

boardfish commented Feb 19, 2025

Gave this a day's work while my internet was down. I've gotten to a point where all config lives on Rails.application.config.view_component, which I think is an ideal starting point. From here, I need to:

  • review view_component_paths – it looks like we might need two variables, one for the default path for the generator to use and another for the lookup paths for components themselves that we remove from Base#virtual_path.
  • move test_controller into component-local config as possibly the only local option so far.
    I've started looking into this, but I'm not sure there's a way to implement what's described here without causing significant friction or getting magical. Right now vc_test_controller doesn't need to know which component it's dealing with – are we okay with that potential?

I think more options need to be component-local, but there are cases like the above where it's not possible without more changes to how ViewComponent is used...

  • make sure local config is inheritable – it's behaving unusually. Looks like I'm just duplicating things that ActiveSupport::Configurable already does, but since config is reserved as something that's apparently necessary for forms, we'll probably need to reimplement Configurable under a different endpoint (component_configuration?).
  • Make sure configs (ViewComponent::ApplicationConfig and component-local config) call compile_methods! – seems necessary as an optimization to avoid method_missing.

That hopefully gets us to the stage where everything works, but from there I'll need to:

  • potentially look at trying this out in a gem with someone who's been affected to understand whether this really does unblock things for gems/engines that depend on ViewComponent
  • update the documentation in this PR to lock in the changes, assuming everything's verified to be in the right place
  • create a set of RuboCop cops with autocorrect (or a generator?) to ease the migration
  • add deprecation warnings to v3

@boardfish boardfish changed the title Make most configuration local to component instances Reorganise configuration across Rails app and component classes Feb 19, 2025
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.

1 participant