Render collections #274

tclem · 2020-03-24T16:41:27Z

Fixes #21.

Introduces collection rendering for view components with the following API:

<%= render(ProductComponent.collection(@products)) %>

Worked with @joelhawksley on the initial sketch of this with the goal of having this API feel very similar to the standard Rails partial rendering of collections. Opening early for feedback, there are still a few open TODOs.

TODO

Sort out validating :as
Update CHANGELOG

Co-Authored-By: Joel Hawksley <[email protected]>

rainerborene · 2020-03-25T03:16:58Z

lib/view_component/base.rb

+      as = as_variable(@options) || :item
+      args = @options.except(:items, :as)
+
+      @options[:items].map do |item|


I don't think that is the optimal way to render large collections. I suggest creating a string buffer and concatenate each rendered component instead of creating an array, then joining it later. Look for some inspiration on ActionView::OutputBuffer class and see how it works under the hood. :)

Looks like ActionView::OutputBuffer < ActiveSupport::SafeBuffer < String (see buffers.rb, and output_safety.rb) and ActionView::RenderedCollection does map/join as well. We can definitely make this use a buffer, but is that optimal? Do we want to optimize for time? If so, a naive benchmark tells me that join is going to be faster.

require "benchmark" def test_benchmark n = 1000000 Benchmark.bmbm do |x| foo = [1] * n buffer = ActionView::OutputBuffer.new x.report("using ActionView::OutputBuffer") do foo.each do buffer << "foobar" end string = buffer.html_safe end x.report("using map and join") do string = foo.map do "foobar" end.join end end end

user system total real using ActionView::OutputBuffer 0.449913 0.004264 0.454177 ( 0.455215) using map and join 0.085798 0.002497 0.088295 ( 0.088758)

I'm new to rails internals, is there another builder like object besides OutputBuffer? I was also a little surprised that render_in had to return anything as I was expecting some internal buffer to be getting appended to as things are rendered. Is it possible that some of this is just for inline test rendering?

Your benchmark isn't really apples to apples because you're doing html_safe in one and not the other. I think we'd want to do a benchmark of a real case using the rendering of the view component instead of test strings. My gut says that Rails must be doing html_safe etc. inside the loop.

Imagine that with rendering components it looks more like this - note the new html_safe in the join case:

require "benchmark" def test_benchmark n = 1000000 Benchmark.bmbm do |x| foo = [1] * n buffer = ActionView::OutputBuffer.new x.report("using ActionView::OutputBuffer") do foo.each do buffer << "foobar" end string = buffer.html_safe end x.report("using map and join") do string = foo.map do "foobar".html_safe end.join end end end

then

user system total real using ActionView::OutputBuffer 0.373818 0.002911 0.376729 ( 0.377495) using map and join 0.821182 0.017310 0.838492 ( 0.840472)

Yup, you're right, both cases need to call html_safe (which is what's happening at the end of render_in), but I think they both need to call it inside the loop. The benchmark you pasted has the OutputBuffer case doing html_safe once at the end and the map/join case doing html_safe on each iteration. Let's make them the same.

I think we'd want to do a benchmark of a real case using the rendering of the view component instead of test strings.

Love this suggestion! Here's an updated benchmark that actually calls render_inline:

require "benchmark" def test_benchmark n = 10000 @product = OpenStruct.new(title: "Bye") Benchmark.bmbm do |x| foo = [1] * n buffer = ActionView::OutputBuffer.new x.report("using ActionView::OutputBuffer") do foo.each do buffer << render_inline(ProductComponent.new(product: @product, extra: "abc")) end end x.report("using map and join") do foo.map do render_inline(ProductComponent.new(product: @product, extra: "abc")) end.join end end end

user system total real using ActionView::OutputBuffer 1.427219 0.023809 1.451028 ( 1.461946) using map and join 1.152875 0.026003 1.178878 ( 1.204991)

I didn't dig too far, but array joins in ruby are implemented in c and appear to be specialized for strings.

Re-running benchmarks with the knowledge that map/join will require a final html_safe. The OutputBuffer version doesn't need this because it's already a safe buffer.

map/join still wins by a little.

require "benchmark" def test_benchmark n = 10000 @product = OpenStruct.new(name: "Soap") Benchmark.bmbm do |x| foo = [1] * n buffer = ActionView::OutputBuffer.new x.report("using ActionView::OutputBuffer") do foo.each do buffer << render_inline(ProductComponent.new(product: @product, notice: "abc")) end end x.report("using map and join") do foo.map do render_inline(ProductComponent.new(product: @product, notice: "abc")) end.join.html_safe end end end

user system total real using ActionView::OutputBuffer 1.543178 0.028958 1.572136 ( 1.573645) using map and join 1.285421 0.025860 1.311281 ( 1.312286)

lib/view_component/base.rb

rainerborene · 2020-03-25T03:28:21Z

I didn't like the collection method name but that is subjective. I suggest following the same pattern of render view helper and stick with the same argument names like collection and as (which is already in use). Maybe wrap_all, build_all or something like that makes more sense. Other than that, all good!

tclem · 2020-03-25T21:03:24Z

@rainerborene, thank you so much for the review!

I didn't like the collection method name but that is subjective.

👍 I made another attempt here, curious to hear what you think about the naming. API looks like this now:

# Verbose version
render(ProductComponent.all(collection: @products, as: :product, foo: foo))

# Does the same thing
render(ProductComponent.all(@products, foo: foo))

rainerborene · 2020-03-26T09:59:20Z

Naming is hard. One pattern that I see emerging from Rails codebase is verb + _all like upsert_all, destroy_all, insert_all, touch_all, and so on. Using only all is vague and does not reveal any intention IMO. I suggest using the locals attribute to pass in arbitrary local variables as well. See more here.

jonspalmer

Great work. Love the direction this is going and looks like you've broken the back of making this work.

A few comments/suggestions about the API that we're exposing.

lib/view_component/collection.rb

test/view_component/view_component_test.rb

jonspalmer · 2020-03-26T11:39:31Z

test/view_component/view_component_test.rb

+    assert_selector("h1", count: 2)
+  end
+
+  def test_render_collection_minimal_specify_as


What's the case your guarding against with this test?

test/view_component/view_component_test.rb

jonspalmer · 2020-03-26T11:46:34Z

lib/view_component/base.rb

@@ -14,6 +15,11 @@ class Base < ActionView::Base
    class_attribute :content_areas, default: []
    self.content_areas = [] # default doesn't work until Rails 5.2

+    # Render a component collection.
+    def self.all(*args)


Would it be better to call this collection?

It was: #274 (comment)

I'm fine with either, but have a slight preference for all as I think:

.all(collection: reads better than .collection(collection:.

my argument would be we shouldn't need a collection: argument. My proposal is

MyComponent.with_collection(@products, .....)

I'm into the idea of with_collection and requiring the first argument to be the collection (something that can to_ary) and then dropping all the other special kwargs (like as: and collection:).

And I think we could take or leave the with_ prefix:

MyComponent.collection(@products, extra: foo)

MyComponent.with_collection(@products, extra: foo)

lib/view_component/collection.rb

jonspalmer · 2020-03-26T12:03:44Z

Naming is hard. One pattern that I see emerging from Rails codebase is verb + _all like upsert_all, destroy_all, insert_all, touch_all, and so on. Using only all is vague and does not reveal any intention IMO. I suggest using the locals attribute to pass in arbitrary local variables as well. See more here.

Naming: how about with_collection which would be very Rails.
WRT the locals comment I'm not sure I understand. Are you suggesting nesting the args under locals? That feels more awkward to me.

rainerborene · 2020-03-26T13:18:17Z

Naming: how about with_collection which would be very Rails.
WRT the locals comment I'm not sure I understand. Are you suggesting nesting the args under locals? That feels more awkward to me.

@jonspalmer If we want this library to be merged on Rails in future, we need to stay as close as possible to Rails internals.

jonspalmer · 2020-03-26T14:04:20Z

@jonspalmer If we want this library to be merged on Rails in future, we need to stay as close as possible to Rails internals.

Sure that's a good guiding principal however we should step back and understand the reasoning for the rails pattern. Passing locals to a partial is something like this

<h1>New zone</h1>
<%= render partial: "form", locals: {zone: @zone} %>

The value of locals in this use case is to distinguish between a) the options provided to the render method b) the set of parameters that will be passed to the partial

In our case I'm proposing that we make as something you define on the Component class and then simplify the signature to just

def self.with_collection(collection, **component_args)

There is no confusion with that API.

It's also nice because it makes the calling mostly "symmetric" when using a collection or not. i.e.

<%= render ProductComponent.new(product: @product, foo: 1, bar: 2) %>

<%= render ProductComponent.with_collection(@products, foo: 1, bar: 2) %>

jonspalmer · 2020-03-26T14:05:13Z

Can we add an 'integration' test of this actually running in the test app?

Must pass collection object directly, no more special keyword args

tclem · 2020-03-26T17:17:43Z

Alright, I made another pass at the API and addressed a bunch of specific feedback. Thanks for the review and some great ideas @jonspalmer! I did add a single integration level test too which was a good idea because that demonstrated the need to use html_safe (I'll re-run my benchmarks next).

Here's the current state of the API:

ProductComponent.with_collection(@products, foo: foo, …)

And instead of having any special keyword args, if a component needs to have a different parameter name you can do that like so (otherwise in this case the name would be product_coupon:):

class ProductCouponComponent < ViewComponent::Base
  with_collection_parameter :item

  def initialize(item:)
    @item = item
  end
end

This also means that all additional hash/keyword args are passed directly to the component's constructor which I think is quite nice.

joelhawksley · 2020-03-26T17:36:41Z

CHANGELOG.md

@@ -1,5 +1,11 @@
 # master

+# v2.1.0
+
+* Support rendering collection (e.g., `render(MyComponent.all(@items))`).


Just dropping a note here to make sure we update this to reflect whatever API we land on ❤️

test/view_component/view_component_test.rb

rainerborene · 2020-03-26T21:39:19Z

Great work @tclem. Make sure to rebase before merging this PR. 👊

tclem · 2020-03-26T21:50:48Z

lib/view_component/version.rb

@@ -3,7 +3,7 @@
 module ViewComponent
  module VERSION
    MAJOR = 2
-    MINOR = 0
+    MINOR = 1


@joelhawksley would you prefer to do version bump and release in a separate PR? Just read the contributing guidelines...

I would, if you don't mind removing this change 😄

joelhawksley

Wonderful work here ❤️

Thanks for all the thoughtful feedback @jonspalmer @rainerborene

test/view_component/view_component_test.rb

README.md

Co-Authored-By: Joel Hawksley <[email protected]>

This reverts commit 4364c69.

CHANGELOG.md

tclem and others added 7 commits March 24, 2020 07:21

Sketch out API for rendering collections

e1f0bce

Co-Authored-By: Joel Hawksley <[email protected]>

Final newlines are nice

c8997b1

Rename template

698fa7d

Handle :as, fill out test coverage

0fe965f

Test out not using :as

7aaef37

Merge remote-tracking branch 'origin/master' into render-collections

d6f435e

Name the ivar item for clarity

6df4749

rainerborene reviewed Mar 25, 2020

View reviewed changes

lib/view_component/base.rb Outdated Show resolved Hide resolved

tclem added 10 commits March 25, 2020 12:25

Change up the api to .all(collection: ...)

6750a3a

Move default kw to as_variable

6d96256

Auto generate item name

6a3e54c

Handle passing collection object directly

492c711

Duplicative of ProductCoupon

85806b4

Just change the listing order

a41527b

Move ViewComponent:Collection to a dedicated file

4b7e367

Draft a changelog entry

c872eae

Add myself to the readme and tidy contributors

2255967

Add space after comma

606d568

joelhawksley requested a review from rainerborene March 25, 2020 22:20

jonspalmer reviewed Mar 26, 2020

View reviewed changes

tclem added 2 commits March 26, 2020 07:59

Use the component class name directly

9baecd1

More specific asserts for extra arguments

556de9e

tclem added 9 commits March 26, 2020 08:27

ViewComponent::Collection constructor can be private

f897365

Be defensive against component changing the args hash

3793dd3

s/all/with_collection

f294f99

Iterate on the api

44f326d

Must pass collection object directly, no more special keyword args

Bring tests up-to-date

9f1bc8a

Express these as list items

84a6495

Wire up an integration test

6c3e7ee

html_safe is required for rendered content to show up

7196bef

Different versions of ruby show slightly different errors here

1b186fe

joelhawksley reviewed Mar 26, 2020

View reviewed changes

tclem added 2 commits March 26, 2020 11:03

Update api in changelog entry

340122b

Bump version to 2.1.0

4364c69

jonspalmer reviewed Mar 26, 2020

View reviewed changes

test/view_component/view_component_test.rb Outdated Show resolved Hide resolved

No need to use ivars in these tests

6612f8b

tclem commented Mar 26, 2020

View reviewed changes

joelhawksley approved these changes Mar 26, 2020

View reviewed changes

test/view_component/view_component_test.rb Outdated Show resolved Hide resolved

README.md Show resolved Hide resolved

tclem and others added 4 commits March 26, 2020 15:53

Update test/view_component/view_component_test.rb

5a13abe

Co-Authored-By: Joel Hawksley <[email protected]>

Revert "Bump version to 2.1.0"

a1b85fc

This reverts commit 4364c69.

Add some documentation to the readme

93b3cdc

Didn't mean to replace these

b09ca46

joelhawksley reviewed Mar 27, 2020

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

Update CHANGELOG.md

c32d066

joelhawksley merged commit e40707a into master Mar 27, 2020

joelhawksley deleted the render-collections branch March 27, 2020 16:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Render collections #274

Render collections #274

tclem commented Mar 24, 2020 •

edited by joelhawksley

Loading

rainerborene Mar 25, 2020

tclem Mar 25, 2020

jonspalmer Mar 26, 2020

tclem Mar 26, 2020

tclem Mar 26, 2020

rainerborene commented Mar 25, 2020 •

edited

Loading

tclem commented Mar 25, 2020

rainerborene commented Mar 26, 2020

jonspalmer left a comment

jonspalmer Mar 26, 2020

jonspalmer Mar 26, 2020

joelhawksley Mar 26, 2020

jonspalmer Mar 26, 2020

tclem Mar 26, 2020

tclem Mar 26, 2020

jonspalmer commented Mar 26, 2020

rainerborene commented Mar 26, 2020

jonspalmer commented Mar 26, 2020

jonspalmer commented Mar 26, 2020

tclem commented Mar 26, 2020

joelhawksley Mar 26, 2020

rainerborene commented Mar 26, 2020

tclem Mar 26, 2020

joelhawksley Mar 26, 2020

joelhawksley left a comment

Render collections #274

Render collections #274

Conversation

tclem commented Mar 24, 2020 • edited by joelhawksley Loading

TODO

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rainerborene commented Mar 25, 2020 • edited Loading

tclem commented Mar 25, 2020

rainerborene commented Mar 26, 2020

jonspalmer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonspalmer commented Mar 26, 2020

rainerborene commented Mar 26, 2020

jonspalmer commented Mar 26, 2020

jonspalmer commented Mar 26, 2020

tclem commented Mar 26, 2020

Choose a reason for hiding this comment

rainerborene commented Mar 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joelhawksley left a comment

Choose a reason for hiding this comment

tclem commented Mar 24, 2020 •

edited by joelhawksley

Loading

rainerborene commented Mar 25, 2020 •

edited

Loading