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

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

Draft
wants to merge 24 commits into
base: introduce-component-local-config
Choose a base branch
from

Conversation

boardfish
Copy link
Collaborator

Based on #1987

Aims to have strict_helpers_enabled? able to be set at an application level by Rails.application.config.view_component without affecting the base value for the setting stored on ViewComponent::Base, which may be consumed by gems.


if child.superclass == ViewComponent::Base
child.define_singleton_method(:config) do
@@config ||= Rails.application.config.view_component.inheritable_copy
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll leave a comment to this effect, but ViewComponent::Base starts out with the defaults. These overrides allow subclasses to inherit from Rails.application.config to start with and cascade down from there.

To achieve our goal of making sure that changes to config don't broadly interfere with gems that want to integrate with ViewComponent, we should probably make it easier here for folks to override config with a base config.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this'll be done by gems having their own ApplicationComponent equivalent now that directly inherits from Base.

@boardfish boardfish force-pushed the strict-helpers-component-local-config branch from 56f28b9 to 001de33 Compare October 6, 2024 11:04
@boardfish
Copy link
Collaborator Author

Made some good progress today, but I'm struggling to diagnose the failure with the generator. Looks like it's not creating anything at all, but the generator does finish and run its create_preview_file method in full.

I should probably also remove Config.current in this PR.

@reeganviljoen
Copy link
Collaborator

@boardfish sorry for not being supper active, I have just been extemly busy, I will try my best to make time to look at this this week

@boardfish
Copy link
Collaborator Author

No problem at all! I think this is almost ready as far as getting back to parity with what we have already - just need to diagnose and fix that failing test.

Because this touches config as a whole, I think there's probably wider discussion necessary about how to roll this out – it's a breaking change, and I think it might be difficult to fully preserve the old behaviour as an option.

@joelhawksley
Copy link
Member

It's a breaking change, and I think it might be difficult to fully preserve the old behaviour as an option.

I think that approach makes sense, with the plan to remove the old behavior in v4 ❤️

@boardfish
Copy link
Collaborator Author

That's good @joelhawksley – so just to be totally clear, the current state of this PR means that folks still interface with global config via Rails.application.config.view_component, but can no longer do so via ViewComponent::Base.config. Is that something we're happy to include in a v3 minor release?

I guess as far as v4, we can try and draw distinctions between global config (Rails.application.config.view_component and component-level config ApplicationComponent.component_config) in some follow-ups that make some options component-specific. Does that sounds good?

@boardfish boardfish force-pushed the strict-helpers-component-local-config branch 2 times, most recently from 8380bd0 to 8c737f6 Compare October 8, 2024 12:47
ViewComponent::Base itself will always use the defaults. Any classes inheriting from ViewComponent::Base will cascade config down using InheritableOptions, only specifying any overrides.
Generate options being on their own "layer of config is currently unresolved - it might be that config needs to be a new object inheriting from InheritableOptions that has method accessors for everything in that namespace.

This was initially written to support extracting the incoming strict_helpers_enabled? option, but applies to everything.
@boardfish
Copy link
Collaborator Author

boardfish commented Oct 8, 2024

I've opened #2124 to introduce component-local config in isolation of Reegan's original PR, but I still need to fix the tests.

EDIT: It's green now – @reeganviljoen, it might be a good idea for you to rebase #1987 onto #2124 to separate the two changes. There are some things I've needed to drop from my commits here because that PR won't actually create component-local config, only reshuffle things to make it a possibility.

reeganviljoen and others added 6 commits October 8, 2024 16:59
ViewComponent::Base itself will always use the defaults. Any classes inheriting from ViewComponent::Base will cascade config down using InheritableOptions, only specifying any overrides.
Generate options being on their own "layer of config is currently unresolved - it might be that config needs to be a new object inheriting from InheritableOptions that has method accessors for everything in that namespace.

It seems like this solution is now applicable to everything, not just strict_helper_enabled?
@boardfish boardfish force-pushed the strict-helpers-component-local-config branch from 8c737f6 to e8bc4ef Compare October 8, 2024 16:00
@boardfish boardfish changed the base branch from main to introduce-component-local-config October 8, 2024 16:00
@boardfish boardfish mentioned this pull request Oct 9, 2024
@boardfish boardfish force-pushed the introduce-component-local-config branch from 161cee3 to 6378768 Compare October 10, 2024 11:18
@boardfish boardfish force-pushed the introduce-component-local-config branch 4 times, most recently from 2dcc88d to 62e0960 Compare October 21, 2024 10:31
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.

3 participants