288 support wicked pdf

[johannesengl]

Summary

This PR aims to solve conflicts with other gems also monkey-patching the ActionView and ActionController render method by giving users an option config to disable the monkey patch and instead of using the render method in view component templates to use the render_component helper.

In < Rails 6.1 the method render_component gets included in ActionView and users can render view components in the following way:

<%= render_component Component.new(message: "bar") %>

The method render_component and render_component_to_string also get included in ActionController

Related Issue
#288

[joelhawksley]

@johannesengl thanks for the PR! I've marked it as ready for review. Would you be willing to resolve the merge conflicts?

[johannesengl]

@johannesengl thanks for the PR! I've marked it as ready for review. Would you be willing to resolve the merge conflicts?

@joelhawksley Sure no problem. I just resolved the conflicts.

When should I open a PR in draft mode and when should it be set to ready for review? The current PR introduces two new tests that are failing and I opened the PR so we could discuss how to solve this issue:
#358 (comment)

[joelhawksley]

Looking good! Might you add some README updates, including your name on the contributor list, along with a CHANGELOG entry?

When should I open a PR in draft mode and when should it be set to ready for review?

Once want the code reviewed 😄. I moved this to ready to review as I felt it was directionally correct ❤️

[rmacklin]

Disclaimer: This is totally not a big deal and I'm happy to stick with Joel's preference for this project.

IMHO, the draft PR state indicates "not ready for merge" (which GitHub helpfully enforces!) but doesn't make a claim about whether it can/should be reviewed; since it's possible to explicitly request a review on a draft PR, that action is the indication that you'd like a review on the PR as it currently stands. With that interpretation, I consider clicking the "Ready for review" button to mean "Ready for review and potentially ready for merge". (All that button really does is take it out of the draft state, which allows the PR to be merged once the other required conditions (status checks/required reviews/no merge conflicts) are met.)

There are times when I'll mark a PR as a draft even if CI has passed, it's been reviewed and approved, and I don't plan on making any further changes to the PR. I do that when I want to prevent it from being merged until something else is done - for example, the PR might not be truly shippable until another system/database/etc. has been updated.

[joelhawksley]

Looks good! Can you add an entry to CHANGELOG and update the README to include your info?

[johannesengl]

Looks good! Can you add an entry to CHANGELOG and update the README to include your info?

@joelhawksley Sure. Just updated the CHANGELOG and README: 685d402

[johannesengl]

@joelhawksley

I would love to discuss a bit more in detail how we would like to setup a second dummy app in our test suit.

Do we want to have only one process running our test suite and then in the test_helper initialize two dummy apps?
I tried this out and pushed a new branch: https://github.com/johannesengl/view_component/tree/second-dummy-app-same-process/test

This approach fails at the point of initializing the second rails application. When executing Dummy2::Application.initialize! I get an error wrong argument type NilClass (expected Proc/Method/UnboundMethod) (TypeError)

Another option would be to invoke two rake tasks and have two folders for reach app including test files. I tried this approach and pushed it to this branch:
https://github.com/johannesengl/view_component/tree/second-dummy-app/test

The issue here is that we also have unexpected behavior when running tests and passing a specific path to a test file. Furthermore, the second run will override the first test coverage reports.

Also happy to jump on a call to discuss this further.

[rmacklin]

Furthermore, the second run will override the first test coverage reports.

SimpleCov declares support for merging of coverage across test suites. Have we tried this setup:
https://github.com/colszowka/simplecov/blob/v0.18.5/README.md#using-simplecov-for-centralized-config

[joelhawksley]

@johannesengl this is a tricky one! I paired with my colleague @seejohnrun on this, and he recommended that we look into using the build_app pattern Rails uses to test similar functionality:

https://github.com/rails/rails/blob/e4b6c719cd1df3dd2bf16645dbe7780dd541ec78/railties/test/isolation/abstract_unit.rb#L99

Perhaps you could look into going that route? Let me know what you think.

Just a note: I'm going to be away from my computer for the next week, so I'll plan to catch up on this PR on or around June 29th 😄

[johannesengl]

@johannesengl this is a tricky one! I paired with my colleague @seejohnrun on this, and he recommended that we look into using the build_app pattern Rails uses to test similar functionality:

https://github.com/rails/rails/blob/e4b6c719cd1df3dd2bf16645dbe7780dd541ec78/railties/test/isolation/abstract_unit.rb#L99

Perhaps you could look into going that route? Let me know what you think.

Just a note: I'm going to be away from my computer for the next week, so I'll plan to catch up on this PR on or around June 29th 😄

@joelhawksley Welcome back. I hope you had a nice break :) Lets maybe align on what we want to archive and also rethink different options.

Option always include render_component
We decided to only add the render_component helper if render_monkey_patch_enabled = false. If we would be willing to add render_component in any case we could test the functionality without the need for a second dummy app.
However, we would still not be able to write a test making sure that the render monkey patches are not included if render_monkey_patch_enabled config is set to false. What do you think regarding this option?

Option several rake tasks
Current issues:

• Combine simplecov reports
• Find fix for making it still possible on passing a TEST parameter to test rake task

Option Booting up new rails app based on abstract_unit.rb used in rails testing
Just to clarify. We want to boot up a second rails app in the same process using the build_app pattern right?
I played around with the abstract_unit code. What it does is it boots up rails from an empty state by including some modules in ActiveSupport::TestCase. This opposes from what we have since we already initialized one dummy rails app.
The build_app method actually invokes the rails new generator. I assume more fitting for us would be the make_basic_app method. I was able to archive that the view component engine got executed a second time with the new config options which led to including render_component. However the render monkey patches were still included in ActionView and ActionCntroller.

I was able to run the following test case in isolation successfully. When running the entire test suit other tests however failed.

  test "rendering component using the render_component helper" do
    make_basic_app do |app|
      app.config.view_component.render_monkey_patch_enabled = false    
    end

    assert !Rails.application.config.view_component.render_monkey_patch_enabled

    get "/render_component"
    assert_includes last_response.body, "bar"

    get "/controller_inline_render_component"
    assert_includes last_response.body, "bar"

    get "/controller_to_string_render_component"
    assert_includes last_response.body, "bar"
  end

I would love to maybe pair with someone on this to move this PR forward.

[rmacklin]

Option always include render_component

I would support always providing render_component. Being explicit and using render_component would offer a minor optimization since it jumps straight to https://github.com/rails/rails/blob/cf1881237a3b40c6232cb321bed4f03ccc897cf5/actionview/lib/action_view/helpers/rendering_helper.rb#L43 without evaluating the case or the if, so it could be useful even in Rails 6.1.

[joelhawksley]

I would support always providing render_component. [...]

I'm wary of going this route. I care a lot about the APIs we provide by default, and I'd much prefer that we use the Rails render call stack in 6.1.

Option Booting up new rails app based on abstract_unit.rb used in rails testing

This looks directionally correct. Let's pair on getting this resolved ❤️

[johannesengl]

I would support always providing render_component. [...]

I'm wary of going this route. I care a lot about the APIs we provide by default, and I'd much prefer that we use the Rails render call stack in 6.1.

Option Booting up new rails app based on abstract_unit.rb used in rails testing

This looks directionally correct. Let's pair on getting this resolved ❤️

After having paired on the Option of booting up new rails app based on abstract_unit.rb with @joelhawksley we decided not to further pursue this direction but rather always include render_component in < rails 6.1.
The option of rendering components via render_component is only given for better backward compatibility and therefore does not justify are more complex test setup using the build_app pattern from rails.

[joelhawksley]

@johannesengl thank you SO much for your work on this PR! I look forward to your next PR :)

[johannesengl]

@johannesengl thank you SO much for your work on this PR! I look forward to your next PR :)

@joelhawksley It was my pleasure. I really enjoyed the collaboration with you. Thanks for making this a great first open-source experience! :)