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

Replace helpers with explicit api in v4 #1848

Closed
Tracked by #1855
reeganviljoen opened this issue Sep 11, 2023 · 7 comments · Fixed by #1860
Closed
Tracked by #1855

Replace helpers with explicit api in v4 #1848

reeganviljoen opened this issue Sep 11, 2023 · 7 comments · Fixed by #1860

Comments

@reeganviljoen
Copy link
Collaborator

          Thanks for the good discussion above y'all, I generally agree that we should keep it `helpers` to stay closer to Rails naming and conventions, but I think this spurred some great conversation. I love the idea of adding a "did you mean" too!

Personally, for V4 I'd want us to consider removing helpers as-is and try to prevent folks from includeing helpers onto components in favor of a more explicit API like:

class MyComponent < ViewComponent::Base
  use_helper :current_user
end

The thought being that it's much more explicit about where your data dependencies are coming from, and mocking them in unit tests is significantly easier. I don't have any plans to push that forward at the moment, but just something I've been thinking on.

Originally posted by @BlakeWilliams in #1836 (comment)

@reeganviljoen reeganviljoen changed the title Thanks for the good discussion above y'all, I generally agree that we should keep it helpers to stay closer to Rails naming and conventions, but I think this spurred some great conversation. I love the idea of adding a "did you mean" too! Replace helpers with explicit api in v4 Sep 11, 2023
@reeganviljoen
Copy link
Collaborator Author

@boardfish @BlakeWilliams @Spone @camertron I closed the previous helpers issue and created this issue to discuss @BlakeWilliams suggestion

@reeganviljoen
Copy link
Collaborator Author

@BlakeWilliams since you gave us the comment on this I was thinking maybe you can give us a bit of an explanation, your thoughts, some concerns, benefits, etc about this possible change

@camertron camertron mentioned this issue Sep 20, 2023
7 tasks
@camertron
Copy link
Contributor

@reeganviljoen we discussed this internally and I think we're on board to give this new API a try. What we have done in the past for changes like this is to define a module that consumers can opt-into using, eg:

class MyComponent < ViewComponent::Base
  include ViewComponent::HelpersApi

  use_helper :current_user
end

It would also be nice to come up with an API that allows these helper methods to be stubbed/mocked in the test environment. Any ideas there? We came up with this, but maybe there's a better option:

class Test < Minitest::Test
  def test_something
    with_helpers(:current_user, -> { User.new }) do
      render_inline(...)
    end
  end
end

Or maybe:

class Test < Minitest::Test
  def test_something
    renderer = TestRenderer.new
    renderer.with_helper(:current_user) do
      User.new
    end
    renderer.render(TestComponent.new(...))
  end
end

Thoughts?

@reeganviljoen
Copy link
Collaborator Author

@camertron I agree with the above with a single addition, maybe adding a deprecation warning in when helpers is used similar to what happened with require: engine in v3

@reeganviljoen
Copy link
Collaborator Author

@camertron I especially like the helper stub

@camertron
Copy link
Contributor

camertron commented Sep 21, 2023

@reeganviljoen yeah, we would definitely add a deprecation warning if/when we think the feature has had long enough to mature. I wouldn't want to add the deprecation warning right away tho, since there's a chance we'd remove the new API before v4 if nobody likes it.

@reeganviljoen
Copy link
Collaborator Author

@camertron I will be working towards a working example this weekend, then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants