`.with_collection` supports Components with splatted KeywordArgs #1560

zspencer · 2022-10-24T17:51:38Z

What are you trying to accomplish?

I'd like to be lazy and not define initialize on every view component when they inherit from a parent class that accepts keyword arguments in order for the component to work with `with_collection.

For example:

class ApplicationComponent < ViewComponent::Base
  def initialize(**kwargs)
    super
    kwargs.each do |k, v|
      send(:"#{k}=", v)
    end
  end
end

class ProductComponent < ApplicationComponent
  attr_accessor :product
end

class OrderComponent < ApplicationComponent
  attr_accessor :order
end

However, ViewComponent doesn't recognize that a keyword args is an implicit def initialize(product:) or def initialize(order:); which causes it to raise an ArgumentError for an unaccepted parameter.

What approach did you choose and why?

I made it so Components with Keyword Args in their initialize pass the initialze parameter name check. This seemed like a small enough change?

Anything you want to highlight for special attention from reviewers?=

Are there side-effects of this I'm not aware of?
Does this create subtle design pressure that encourages cruft?

In a few projects, I wind up using keyword args on a Base class so that I don't have to define an `initialize` on every child class. For example: ```ruby class ApplicationComponent < ViewComponent::Base def initialize(**kwargs) super kwargs.each do |k, v| send(:"#{k}=", v) end end end class ProductComponent < ApplicationComponent attr_accessor :product end class OrderComponent < ApplicationComponent attr_accessor :order end ``` However, ViewComponent doesn't recognize that a keyword args is an implicit `def initialize(product:)` or `def initialize(order:)`; which causes it to raise an `ArgumentError` for an unaccepted parameter. This causes Components with Keyword Args in their initialize to pass the initialze parameter name check. Open Questions: - [ ] Are there side-effects of this I'm not aware of? - [ ] Does this create subtle design pressure that encourages cruft? Thoughts?

zspencer · 2022-10-24T19:04:22Z

Woops, I must have forgotten to include the empty call method definition when I pulled out the class. One moment...

docs/CHANGELOG.md

joelhawksley · 2022-11-02T17:23:54Z

test/sandbox/test/collection_test.rb

+      collection = ComponentWithKeywordArgs.with_collection(@products)
+      render_inline(collection)


Suggested change

collection = ComponentWithKeywordArgs.with_collection(@products)

render_inline(collection)

render_inline(ComponentWithKeywordArgs.with_collection(@products))

Can you add an assertion that something is rendered correctly?

Co-authored-by: Joel Hawksley <[email protected]>

joelhawksley · 2022-11-08T14:50:25Z

👋🏻 it looks like you do have a legit failure in CI ❤️

zspencer · 2022-11-08T16:40:54Z

Woops! I ran them locally but must have misread the output. That's what I get for shoveling all my open-source stuff to end-of-business!

I appear to have forgotten that the with_collection leverages the components name to figure out the name of the argument it uses for the collection; rather than the name of the items in the collection (which is how it works in Rails).

zspencer · 2022-11-09T17:15:19Z

Which is to say, I think the dish is ready? I don't understand the PVC error (I am assuming it's a permissions thing?)

jcoyne · 2022-11-09T18:26:34Z

docs/CHANGELOG.md

@@ -10,6 +10,10 @@ nav_order: 5

 ## main

+* `Component.with_collection` supports components that accept keyword arguments.


Can this be changed to "accept splatted keyword arguments"? I believe defined keyword args are not a problem.

That's much clearer!

jcoyne · 2022-11-09T18:27:28Z

lib/view_component/base.rb

@@ -591,7 +591,7 @@ def validate_collection_parameter!(validate_default: false)
        parameter = validate_default ? collection_parameter : provided_collection_parameter

        return unless parameter
-        return if initialize_parameter_names.include?(parameter)
+        return if initialize_parameter_names.include?(parameter) || initialize_parameter_names.include?(:kwargs)


Does this imply that the splatted argument has to have the name kwargs? That is, would this work with def initialize(**other_args)?

An excellent question to which I do not yet have a conclusive answer. I'll update the tests and report back.

Excellent catch! I've updated to check for :kwrest in the flattened initialize parameters!

Thanks for making that change.

Co-authored-by: [email protected]

@camertron

While working on [supporting splatted keyword args](ViewComponent#1560) I discovered that the linter was failing me on changes I didn't make. When I ran standardrb locally, it was quite content, so I was a bit confused. Then I read [.github/workflows/lint.yml] and realized that view_component runs a `bundle update` prior to running standard; which bumps the standardrb version and the corresponding rubocop set. This wasintroduced by @camertron in [e7eec98](ViewComponent@e7eec98) 7+ months ago to fix an error in CI. It's unclear to me why `bundle install` was not working, so I decided that rather than swapping that back I would trust @camertron's instincts and roll-forward with the update.

@camertron

While working on [supporting splatted keyword args](ViewComponent#1560) I discovered that the linter was failing me on changes I didn't make. When I ran standardrb locally, it was quite content, so I was a bit confused. Then I read [.github/workflows/lint.yml] and realized that view_component runs a `bundle update` prior to running standard; which bumps the standardrb version and the corresponding rubocop set. This wasintroduced by @camertron in [e7eec98](ViewComponent@e7eec98) 7+ months ago to fix an error in CI. It's unclear to me why `bundle install` was not working, so I decided that rather than swapping that back I would trust @camertron's instincts and roll-forward with the update. I am not including a change to the Changelog because this seems internal and unnecessary to communicate to package consumers.

zspencer · 2022-11-11T19:35:25Z

Once more into the fray! The lint failures appear to be unrelated to my changes; so I made a patch to resolve those as well: #1576

zspencer · 2022-11-11T22:05:40Z

OK look slike the 3.1 build is not happy will poke at it 🔜

@camertron

While working on [supporting splatted keyword args](#1560) I discovered that the linter was failing me on changes I didn't make. When I ran standardrb locally, it was quite content, so I was a bit confused. Then I read [.github/workflows/lint.yml] and realized that view_component runs a `bundle update` prior to running standard; which bumps the standardrb version and the corresponding rubocop set. This wasintroduced by @camertron in [e7eec98](e7eec98) 7+ months ago to fix an error in CI. It's unclear to me why `bundle install` was not working, so I decided that rather than swapping that back I would trust @camertron's instincts and roll-forward with the update. I am not including a change to the Changelog because this seems internal and unnecessary to communicate to package consumers.

In Ruby 3.1, it appears that the `ViewComponent::Base#initialize` method has an implicit key-rest argument, named `:**`. I thought it may be because we define it with a `def initialize(*); end;` which could be implicitly adding in the rest of the splattable parameter? However, playing around with that syntax doesn't seem to be the cause: ```ruby class A; def initialize(*); end; end class B; end A.instance_method(:initialize).parameters B.instance_method(:initialize).parameters ```

zspencer · 2022-11-14T22:54:03Z

OK, looks like we are back to green on CI; and I have resolved the test failure for ruby 3.1.

lib/view_component/base.rb

joelhawksley · 2022-11-16T19:01:02Z

@zspencer can you fix the merge conflicts? ❤️

zspencer · 2022-11-16T19:05:09Z

@joelhawksley - I have several times at this point... They'll keep coming up when y'all merge other stuff, so for me to do it feels like a dog chasing its tail a bit.

zspencer · 2022-11-16T20:36:09Z

@joelhawksley - You'll need to approve it in order for auto-merge to work, I think.

joelhawksley · 2022-11-16T20:37:40Z

That'll do it! Thanks @zspencer ❤️

zspencer · 2022-11-17T16:02:28Z

@camertron

While working on [supporting splatted keyword args](ViewComponent#1560) I discovered that the linter was failing me on changes I didn't make. When I ran standardrb locally, it was quite content, so I was a bit confused. Then I read [.github/workflows/lint.yml] and realized that view_component runs a `bundle update` prior to running standard; which bumps the standardrb version and the corresponding rubocop set. This wasintroduced by @camertron in [e7eec98](ViewComponent@e7eec98) 7+ months ago to fix an error in CI. It's unclear to me why `bundle install` was not working, so I decided that rather than swapping that back I would trust @camertron's instincts and roll-forward with the update. I am not including a change to the Changelog because this seems internal and unnecessary to communicate to package consumers.

…wComponent#1560) * Initial support for Components with KeywordArgs In a few projects, I wind up using keyword args on a Base class so that I don't have to define an `initialize` on every child class. For example: ```ruby class ApplicationComponent < ViewComponent::Base def initialize(**kwargs) super kwargs.each do |k, v| send(:"#{k}=", v) end end end class ProductComponent < ApplicationComponent attr_accessor :product end class OrderComponent < ApplicationComponent attr_accessor :order end ``` However, ViewComponent doesn't recognize that a keyword args is an implicit `def initialize(product:)` or `def initialize(order:)`; which causes it to raise an `ArgumentError` for an unaccepted parameter. This causes Components with Keyword Args in their initialize to pass the initialze parameter name check. Open Questions: - [ ] Are there side-effects of this I'm not aware of? - [ ] Does this create subtle design pressure that encourages cruft? Thoughts? * Render nothing just to get past the explode boom * Lint lint lint * Standardrb vs Ruby 2.5: FIGHT! * Add Changelog per CI * Update docs/CHANGELOG.md Co-authored-by: Joel Hawksley <[email protected]> * Assert rendering against the collection works * The Kwarg is the Component name 🙈 I appear to have forgotten that the with_collection leverages the components name to figure out the name of the argument it uses for the collection; rather than the name of the items in the collection (which is how it works in Rails). * Clarify that this is for Splatted Keyword Args Co-authored-by: [email protected] * Keyword argument detection is more resilient * Don't count un-named splatted keyword args In Ruby 3.1, it appears that the `ViewComponent::Base#initialize` method has an implicit key-rest argument, named `:**`. I thought it may be because we define it with a `def initialize(*); end;` which could be implicitly adding in the rest of the splattable parameter? However, playing around with that syntax doesn't seem to be the cause: ```ruby class A; def initialize(*); end; end class B; end A.instance_method(:initialize).parameters B.instance_method(:initialize).parameters ``` * Make it actually private Co-authored-by: Joel Hawksley <[email protected]>

@camertron

While working on [supporting splatted keyword args](ViewComponent#1560) I discovered that the linter was failing me on changes I didn't make. When I ran standardrb locally, it was quite content, so I was a bit confused. Then I read [.github/workflows/lint.yml] and realized that view_component runs a `bundle update` prior to running standard; which bumps the standardrb version and the corresponding rubocop set. This wasintroduced by @camertron in [e7eec98](ViewComponent@e7eec98) 7+ months ago to fix an error in CI. It's unclear to me why `bundle install` was not working, so I decided that rather than swapping that back I would trust @camertron's instincts and roll-forward with the update. I am not including a change to the Changelog because this seems internal and unnecessary to communicate to package consumers.

…wComponent#1560) * Initial support for Components with KeywordArgs In a few projects, I wind up using keyword args on a Base class so that I don't have to define an `initialize` on every child class. For example: ```ruby class ApplicationComponent < ViewComponent::Base def initialize(**kwargs) super kwargs.each do |k, v| send(:"#{k}=", v) end end end class ProductComponent < ApplicationComponent attr_accessor :product end class OrderComponent < ApplicationComponent attr_accessor :order end ``` However, ViewComponent doesn't recognize that a keyword args is an implicit `def initialize(product:)` or `def initialize(order:)`; which causes it to raise an `ArgumentError` for an unaccepted parameter. This causes Components with Keyword Args in their initialize to pass the initialze parameter name check. Open Questions: - [ ] Are there side-effects of this I'm not aware of? - [ ] Does this create subtle design pressure that encourages cruft? Thoughts? * Render nothing just to get past the explode boom * Lint lint lint * Standardrb vs Ruby 2.5: FIGHT! * Add Changelog per CI * Update docs/CHANGELOG.md Co-authored-by: Joel Hawksley <[email protected]> * Assert rendering against the collection works * The Kwarg is the Component name 🙈 I appear to have forgotten that the with_collection leverages the components name to figure out the name of the argument it uses for the collection; rather than the name of the items in the collection (which is how it works in Rails). * Clarify that this is for Splatted Keyword Args Co-authored-by: [email protected] * Keyword argument detection is more resilient * Don't count un-named splatted keyword args In Ruby 3.1, it appears that the `ViewComponent::Base#initialize` method has an implicit key-rest argument, named `:**`. I thought it may be because we define it with a `def initialize(*); end;` which could be implicitly adding in the rest of the splattable parameter? However, playing around with that syntax doesn't seem to be the cause: ```ruby class A; def initialize(*); end; end class B; end A.instance_method(:initialize).parameters B.instance_method(:initialize).parameters ``` * Make it actually private Co-authored-by: Joel Hawksley <[email protected]>

daniel-rikowski · 2024-04-27T08:49:00Z

lib/view_component/base.rb

@@ -636,6 +636,11 @@ def iteration_argument_present?

      private

+      def splatted_keyword_argument_present?
+        initialize_parameters.flatten.include?(:keyrest) &&
+          !initialize_parameters.include?([:keyrest, :**]) # Un-named splatted keyword args don't count!


Is there a specific reason for not accepting unnamed splatted arguments? This limitation creates some problems with initializers, which forward their arguments to helpers, e.g. dry-initialize, which uses def initialize(...); __dry_initializer_initialize__(...); end (... = actual Ruby code) or any kind of custom helper, like def initialize(**); do_init(**); end; (Sorry for "gravedigging", I'll open a separate issue, if that is more appropriate...)

I can't remember my exact reasoning, but it was more a limitation-in-my-capacity and not necessarily a limitation-by-design.

@daniel-rikowski @zspencer can we please open up a new issue where this can be discussed further

👋 I'd also be interested in a PR with a failing test for the use case you want 😄

See #2020

@zspencer It seems this was because of a Ruby 3.1 bug 😉

zspencer marked this pull request as draft October 24, 2022 19:04

zspencer added 3 commits October 24, 2022 12:05

Render nothing just to get past the explode boom

0ff2dc5

Lint lint lint

9341fa9

Standardrb vs Ruby 2.5: FIGHT!

fd16273

zspencer changed the title ~~Initial support for Components with KeywordArgs~~ .with_collection supports Components whose initialize method takes KeywordArgs Oct 24, 2022

Add Changelog per CI

47fd21a

zspencer force-pushed the enhancement/support-keyword-args-in-components branch from a8b54b3 to 47fd21a Compare October 24, 2022 19:12

zspencer marked this pull request as ready for review October 24, 2022 19:14

joelhawksley reviewed Nov 2, 2022

View reviewed changes

zspencer and others added 3 commits November 7, 2022 15:49

Update docs/CHANGELOG.md

bad0792

Co-authored-by: Joel Hawksley <[email protected]>

Merge branch 'main' into enhancement/support-keyword-args-in-components

b919c03

Assert rendering against the collection works

33a4daf

zspencer force-pushed the enhancement/support-keyword-args-in-components branch from 49034fe to 33a4daf Compare November 7, 2022 23:56

zspencer marked this pull request as draft November 8, 2022 16:41

The Kwarg is the Component name 🙈

ed460f6

I appear to have forgotten that the with_collection leverages the components name to figure out the name of the argument it uses for the collection; rather than the name of the items in the collection (which is how it works in Rails).

zspencer marked this pull request as ready for review November 9, 2022 17:13

jcoyne reviewed Nov 9, 2022

View reviewed changes

zspencer added 2 commits November 11, 2022 10:41

Clarify that this is for Splatted Keyword Args

2bb7b92

Co-authored-by: [email protected]

Keyword argument detection is more resilient

dc84803

zspencer marked this pull request as draft November 11, 2022 19:07

Merge branch 'main' into enhancement/support-keyword-args-in-components

29de919

zspencer mentioned this pull request Nov 11, 2022

Fix CI for patches by applying the new standardrb rules #1576

Merged

zspencer marked this pull request as ready for review November 11, 2022 19:34

zspencer changed the title ~~.with_collection supports Components whose initialize method takes KeywordArgs~~ .with_collection supports Components whose initialize method takes splatted KeywordArgs Nov 11, 2022

zspencer changed the title ~~.with_collection supports Components whose initialize method takes splatted KeywordArgs~~ .with_collection supports Components with splatted KeywordArgs Nov 11, 2022

zspencer marked this pull request as draft November 11, 2022 22:05

zspencer added 2 commits November 14, 2022 14:11

Merge branch 'main' into enhancement/support-keyword-args-in-components

02c9480

zspencer marked this pull request as ready for review November 14, 2022 22:52

joelhawksley reviewed Nov 15, 2022

View reviewed changes

lib/view_component/base.rb Show resolved Hide resolved

zspencer added 2 commits November 15, 2022 09:32

Make it actually private

0bd5318

Merge branch 'main' into enhancement/support-keyword-args-in-components

914bbf3

Merge branch 'main' into enhancement/support-keyword-args-in-components

ff759e1

joelhawksley enabled auto-merge (squash) November 16, 2022 19:13

joelhawksley approved these changes Nov 16, 2022

View reviewed changes

joelhawksley merged commit 56a6560 into ViewComponent:main Nov 16, 2022

zspencer deleted the enhancement/support-keyword-args-in-components branch November 17, 2022 16:01

daniel-rikowski reviewed Apr 27, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`.with_collection` supports Components with splatted KeywordArgs #1560

`.with_collection` supports Components with splatted KeywordArgs #1560

zspencer commented Oct 24, 2022

zspencer commented Oct 24, 2022

joelhawksley Nov 2, 2022

zspencer Nov 7, 2022

joelhawksley commented Nov 8, 2022

zspencer commented Nov 8, 2022

zspencer commented Nov 9, 2022 •

edited

Loading

jcoyne Nov 9, 2022

zspencer Nov 11, 2022

jcoyne Nov 9, 2022

zspencer Nov 11, 2022

zspencer Nov 11, 2022

jcoyne Nov 11, 2022

zspencer commented Nov 11, 2022

zspencer commented Nov 11, 2022

zspencer commented Nov 14, 2022

joelhawksley commented Nov 16, 2022

zspencer commented Nov 16, 2022

zspencer commented Nov 16, 2022

joelhawksley commented Nov 16, 2022

zspencer commented Nov 17, 2022

daniel-rikowski Apr 27, 2024

zspencer May 2, 2024

reeganviljoen May 2, 2024

joelhawksley May 6, 2024

daniel-rikowski May 7, 2024

		collection = ComponentWithKeywordArgs.with_collection(@products)
		render_inline(collection)

	collection = ComponentWithKeywordArgs.with_collection(@products)
	render_inline(collection)
	render_inline(ComponentWithKeywordArgs.with_collection(@products))

		@@ -10,6 +10,10 @@ nav_order: 5

		## main

		* `Component.with_collection` supports components that accept keyword arguments.

.with_collection supports Components with splatted KeywordArgs #1560

.with_collection supports Components with splatted KeywordArgs #1560

Conversation

zspencer commented Oct 24, 2022

What are you trying to accomplish?

What approach did you choose and why?

Anything you want to highlight for special attention from reviewers?=

zspencer commented Oct 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joelhawksley commented Nov 8, 2022

zspencer commented Nov 8, 2022

zspencer commented Nov 9, 2022 • edited Loading

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

zspencer commented Nov 11, 2022

zspencer commented Nov 11, 2022

zspencer commented Nov 14, 2022

joelhawksley commented Nov 16, 2022

zspencer commented Nov 16, 2022

zspencer commented Nov 16, 2022

joelhawksley commented Nov 16, 2022

zspencer commented Nov 17, 2022

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

`.with_collection` supports Components with splatted KeywordArgs #1560

`.with_collection` supports Components with splatted KeywordArgs #1560

zspencer commented Nov 9, 2022 •

edited

Loading