Allow I18n in render lifecycle

[23tux]

What are you trying to accomplish?

1. Sometimes we call t(".some_key") inside a #render? method. Think of def render? = items.any? and items is a hash of translations
2. To test a components method, I'd like to be able to just do the setup of a component's render lifecycle inside my test. This shortens our test run time, as we don't need to actually render the HTML. And due to the change when the @virtual_path is actually set, I can now test t(..) calls in my helper method, because the teardown hasn't happened yet.

What approach did you choose and why?

• I extracted the setup, actual render, and teardown of the #render_in method into methods.
• I moved the line @view_context.instance_variable_set(:@virtual_path, virtual_path) outside of the @output_buffer.with_buffer do block. I just hope this doesn't break anything else.

Anything you want to highlight for special attention from reviewers?

• I couldn't get the system tests with a real browser to run, but all the other tests are green
• I'm not sure if I can just change the values in assert_allocations

[23tux]

Maybe someone can help with the failing checks, I'm not sure what to do

[finalburn]

@23tux This looks good to me. Those checks are failing on main – I don't think they're blockers to merging this, but because this affects the number of allocations it'd probably be good to make sure those tests pass locally if it's convenient to do so.

[23tux]

@boardfish I added a test helper as well, could you have another look at it? Apart from the integration/system specs that require a browser (as I said, I had some problems with the setup and can't run them), all the tests pass on my machine. I added one commit to make the allocation spec more robust.

And the Lint check also fails, but it doesn't seem to involve my changes.

[joelhawksley]

@23tux thank you for taking the time to make this contribution!

In https://viewcomponent.org/best_practices.html#test-against-rendered-content-not-instance-methods, we say:

ViewComponent tests should use render_inline and assert against the rendered output. While it can be useful to test specific component instance methods directly, it’s more valuable to write assertions against what’s shown to the end user

I stand by that recommendation and thus am hesitant to add any test helpers to enable testing component instance methods. There is a reason https://viewcomponent.org/best_practices.html#most-viewcomponent-instance-methods-can-be-private immediately follows the guidance above!

I am happy to have my mind changed here, but for now I'm going to close this PR in favor of #2511 which cherry-picks your bug fix ❤️

[23tux]

@joelhawksley thanks for your detailed explanation and sorry for the delayed response.

So, let me try to change your mind 😀:

• https://viewcomponent.org/best_practices.html#most-viewcomponent-instance-methods-can-be-private

  • I agree that most methods can and should be private, but then they should not be tested at all.
  • But public methods should be tested.

• Better test performance & test readability

  • I agree that in terms of test coverage, you cover more ground if you only test agains the rendered output.
  • Alas in my experience, this leads to slow component tests over time, especially if you have hundreds of components as we do.
  • Furthermore, one has to rely on using xpath or css selectors to get the desired values out of the HTML, so the tests become harder to read.

• Better debugging

  • Having a clearly defined lifecycle, let's you debug easier when problems occur.
  • The current #render_in method is pretty long and splitting it into logical parts leads to easier understanding of the gem internals.

• Easier to test

  • When you have these lifecycle methods, instance methods can be tested in the same state as they are when rendered in production.
  • This leads to easier testing without unintended side effects.

So, I hope these are enough points that you'd consider merging my PR. I rebased it already on the main branch, as our own app already works with that feature in production.

[joelhawksley]

Alas in my experience, this leads to slow component tests over time, especially if you have hundreds of components as we do.

That's fair, but render_inline is still very fast compared to integration or system tests.

Furthermore, one has to rely on using xpath or css selectors to get the desired values out of the HTML, so the tests become harder to read.

I disagree. If our rendered HTML is difficult to select, that can be a sign that it isn't properly accessible. We lean heavily on using accessibility labels for our UI tests to ensure that screen readers can navigate our pages effectively.

Also, I've yet to document it publicly, but we use a test_selector helper that generates a data attribute for identifying elements in tests when necessary.

--

I appreciate your reasoning, but will keep things as they are for now. If and when a more necessary reason is presented, I'd be happy to reconsider. ❤️

[23tux]

Hmm, I’m sorry to hear that. What about my other points?

That's fair, but render_inline is still very fast compared to integration or system tests.

I ran a quick benchmark, and on my machine a component spec with a render_inline call is, on average, about 3× slower than a spec that simply asserts on a public method. To me, that’s a strong argument in favor of this PR.

Also, don’t you think the code reads cleaner - and is easier to follow - when we have clearly defined lifecycles, even if they’re only used for internal structure? I’d be totally fine with making the lifecycle methods private. In that case, I’d frame this PR not as a new feature, but as a refactoring to improve the internal structure.

[joelhawksley]

I ran a quick benchmark, and on my machine a component spec with a render_inline call is, on average, about 3× slower than a spec that simply asserts on a public method. To me, that’s a strong argument in favor of this PR.

Thinking about this some more, what really underpins my opinion is philosophical: I see ViewComponents as things that accept data and return HTML (or JSON) as their input/output contracts. I do not see them as objects to pass messages to.

Also, don’t you think the code reads cleaner - and is easier to follow - when we have clearly defined lifecycles, even if they’re only used for internal structure? I’d be totally fine with making the lifecycle methods private. In that case, I’d frame this PR not as a new feature, but as a refactoring to improve the internal structure.

I'm generally hesitant to break up long methods into single-use methods, especially when the sub-methods are only used privately. Again, this is more a matter of opinion.