From ead72eb8a7d338a986536ba05baa97cbac46da79 Mon Sep 17 00:00:00 2001 From: Hugo Chantelauze Date: Thu, 26 Oct 2023 16:56:08 +0200 Subject: [PATCH 1/9] wip fix bug --- .../app/components/slotable_test_component.html.slim | 5 +++++ test/sandbox/app/components/slotable_test_component.rb | 7 +++++++ test/sandbox/app/components/tag_component.rb | 10 ++++++++++ test/sandbox/test/slotable_test.rb | 9 +++++++++ 4 files changed, 31 insertions(+) create mode 100644 test/sandbox/app/components/slotable_test_component.html.slim create mode 100644 test/sandbox/app/components/slotable_test_component.rb create mode 100644 test/sandbox/app/components/tag_component.rb diff --git a/test/sandbox/app/components/slotable_test_component.html.slim b/test/sandbox/app/components/slotable_test_component.html.slim new file mode 100644 index 000000000..ed6762ce8 --- /dev/null +++ b/test/sandbox/app/components/slotable_test_component.html.slim @@ -0,0 +1,5 @@ +div(class=root_class) + div(class=class_for("image-container")) + = image_tag image, class: class_for("image") + - if tag? + = tag \ No newline at end of file diff --git a/test/sandbox/app/components/slotable_test_component.rb b/test/sandbox/app/components/slotable_test_component.rb new file mode 100644 index 000000000..ee5cb8692 --- /dev/null +++ b/test/sandbox/app/components/slotable_test_component.rb @@ -0,0 +1,7 @@ +class SlotableTestComponent < ViewComponent::Base + renders_one :tag, TagComponent + + def initialize(image) + @image = image + end +end diff --git a/test/sandbox/app/components/tag_component.rb b/test/sandbox/app/components/tag_component.rb new file mode 100644 index 000000000..b959f9a41 --- /dev/null +++ b/test/sandbox/app/components/tag_component.rb @@ -0,0 +1,10 @@ +class TagComponent < ViewComponent::Base + + def initialize(text) + @text = text + end + + def call + content_tag :span, text, class: "tag" + end +end \ No newline at end of file diff --git a/test/sandbox/test/slotable_test.rb b/test/sandbox/test/slotable_test.rb index 3a931167e..46493e891 100644 --- a/test/sandbox/test/slotable_test.rb +++ b/test/sandbox/test/slotable_test.rb @@ -719,4 +719,13 @@ def test_slot_with_content_shorthand assert component.title.content? end + + def test_slotable_component + render_inline(SlotableTestComponent.new(image: "image.png")) do |component| + component.with_tag(text: "Hello World") + end + end + + assert_selector("span") + end end From be781c02465050d62e9afaab91371088c6d3f4e7 Mon Sep 17 00:00:00 2001 From: Hugo Chantelauze Date: Thu, 26 Oct 2023 19:24:36 +0200 Subject: [PATCH 2/9] wip fix bug --- .../app/components/slotable_test_component.html.slim | 8 +++----- test/sandbox/app/components/slotable_test_component.rb | 4 ---- test/sandbox/app/components/tag_component.rb | 2 +- test/sandbox/test/slotable_test.rb | 5 ++--- 4 files changed, 6 insertions(+), 13 deletions(-) diff --git a/test/sandbox/app/components/slotable_test_component.html.slim b/test/sandbox/app/components/slotable_test_component.html.slim index ed6762ce8..618475b41 100644 --- a/test/sandbox/app/components/slotable_test_component.html.slim +++ b/test/sandbox/app/components/slotable_test_component.html.slim @@ -1,5 +1,3 @@ -div(class=root_class) - div(class=class_for("image-container")) - = image_tag image, class: class_for("image") - - if tag? - = tag \ No newline at end of file +div(class="slotable") + = image_tag("https://placehold.it/100x100") + = tag \ No newline at end of file diff --git a/test/sandbox/app/components/slotable_test_component.rb b/test/sandbox/app/components/slotable_test_component.rb index ee5cb8692..4dbf8511b 100644 --- a/test/sandbox/app/components/slotable_test_component.rb +++ b/test/sandbox/app/components/slotable_test_component.rb @@ -1,7 +1,3 @@ class SlotableTestComponent < ViewComponent::Base renders_one :tag, TagComponent - - def initialize(image) - @image = image - end end diff --git a/test/sandbox/app/components/tag_component.rb b/test/sandbox/app/components/tag_component.rb index b959f9a41..c4da1d33d 100644 --- a/test/sandbox/app/components/tag_component.rb +++ b/test/sandbox/app/components/tag_component.rb @@ -5,6 +5,6 @@ def initialize(text) end def call - content_tag :span, text, class: "tag" + content_tag :span, @text, class: "tag" end end \ No newline at end of file diff --git a/test/sandbox/test/slotable_test.rb b/test/sandbox/test/slotable_test.rb index 46493e891..7d6daf2d8 100644 --- a/test/sandbox/test/slotable_test.rb +++ b/test/sandbox/test/slotable_test.rb @@ -721,11 +721,10 @@ def test_slot_with_content_shorthand end def test_slotable_component - render_inline(SlotableTestComponent.new(image: "image.png")) do |component| + render_inline(SlotableTestComponent.new) do |component| component.with_tag(text: "Hello World") - end end - assert_selector("span") + assert_selector("span.tag", text: "Hello World") end end From 7cbfc683cb5cfde1fed93508c8cae284e8d32c2b Mon Sep 17 00:00:00 2001 From: Hugo Chantelauze Date: Mon, 6 Nov 2023 16:36:38 +0100 Subject: [PATCH 3/9] add RedefinedExistingMethodError error --- lib/view_component/errors.rb | 10 ++++++++++ lib/view_component/slotable.rb | 11 +++++++++-- .../app/components/slotable_test_component.html.slim | 3 --- .../app/components/slotable_test_component.rb | 3 --- test/sandbox/app/components/tag_component.rb | 10 ---------- test/sandbox/test/slotable_test.rb | 12 +++++++----- 6 files changed, 26 insertions(+), 23 deletions(-) delete mode 100644 test/sandbox/app/components/slotable_test_component.html.slim delete mode 100644 test/sandbox/app/components/slotable_test_component.rb delete mode 100644 test/sandbox/app/components/tag_component.rb diff --git a/lib/view_component/errors.rb b/lib/view_component/errors.rb index 36f9ee570..677390bae 100644 --- a/lib/view_component/errors.rb +++ b/lib/view_component/errors.rb @@ -126,6 +126,16 @@ def initialize(klass_name, slot_name) end end + class RedefinedExistingMethodError < StandardError + MESSAGE = + "COMPONENT declares a slot named SLOT_NAME, which conflicts with an existing method.\n\n" \ + "To fix this issue, choose a different slot name." + + def initialize(klass_name, slot_name) + super(MESSAGE.gsub("COMPONENT", klass_name.to_s).gsub("SLOT_NAME", slot_name.to_s)) + end + end + class ReservedSingularSlotNameError < StandardError MESSAGE = "COMPONENT declares a slot named SLOT_NAME, which is a reserved word in the ViewComponent framework.\n\n" \ diff --git a/lib/view_component/slotable.rb b/lib/view_component/slotable.rb index ecb4826a2..7e25f0106 100644 --- a/lib/view_component/slotable.rb +++ b/lib/view_component/slotable.rb @@ -310,6 +310,11 @@ def validate_singular_slot_name(slot_name) raise_if_slot_ends_with_question_mark(slot_name) raise_if_slot_registered(slot_name) + raise_if_method_exists(slot_name) + end + + def raise_if_slot_ends_with_question_mark(slot_name) + raise SlotPredicateNameError.new(name, slot_name) if slot_name.to_s.ends_with?("?") end def raise_if_slot_registered(slot_name) @@ -319,8 +324,10 @@ def raise_if_slot_registered(slot_name) end end - def raise_if_slot_ends_with_question_mark(slot_name) - raise SlotPredicateNameError.new(name, slot_name) if slot_name.to_s.ends_with?("?") + def raise_if_method_exists(slot_name) + if instance_methods.include?(slot_name) + raise RedefinedExistingMethodError.new(name, slot_name) + end end end diff --git a/test/sandbox/app/components/slotable_test_component.html.slim b/test/sandbox/app/components/slotable_test_component.html.slim deleted file mode 100644 index 618475b41..000000000 --- a/test/sandbox/app/components/slotable_test_component.html.slim +++ /dev/null @@ -1,3 +0,0 @@ -div(class="slotable") - = image_tag("https://placehold.it/100x100") - = tag \ No newline at end of file diff --git a/test/sandbox/app/components/slotable_test_component.rb b/test/sandbox/app/components/slotable_test_component.rb deleted file mode 100644 index 4dbf8511b..000000000 --- a/test/sandbox/app/components/slotable_test_component.rb +++ /dev/null @@ -1,3 +0,0 @@ -class SlotableTestComponent < ViewComponent::Base - renders_one :tag, TagComponent -end diff --git a/test/sandbox/app/components/tag_component.rb b/test/sandbox/app/components/tag_component.rb deleted file mode 100644 index c4da1d33d..000000000 --- a/test/sandbox/app/components/tag_component.rb +++ /dev/null @@ -1,10 +0,0 @@ -class TagComponent < ViewComponent::Base - - def initialize(text) - @text = text - end - - def call - content_tag :span, @text, class: "tag" - end -end \ No newline at end of file diff --git a/test/sandbox/test/slotable_test.rb b/test/sandbox/test/slotable_test.rb index 7d6daf2d8..ce94eea0c 100644 --- a/test/sandbox/test/slotable_test.rb +++ b/test/sandbox/test/slotable_test.rb @@ -720,11 +720,13 @@ def test_slot_with_content_shorthand assert component.title.content? end - def test_slotable_component - render_inline(SlotableTestComponent.new) do |component| - component.with_tag(text: "Hello World") + def test_raises_error_on_conflicting_slot_names_with_content + exeption = assert_raises ViewComponent::RedefinedExistingMethodError do + Class.new(ViewComponent::Base) do + renders_one :tag + end end - assert_selector("span.tag", text: "Hello World") + assert_includes exeption.message, "declares a slot named tag" end -end +end \ No newline at end of file From 432bb34d456357d3b1cf03dc985c19f250cb8491 Mon Sep 17 00:00:00 2001 From: Hugo Chantelauze Date: Tue, 7 Nov 2023 09:24:40 +0100 Subject: [PATCH 4/9] add RedefinedExistingMethodError error test with renders_many slot --- test/sandbox/test/slotable_test.rb | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/test/sandbox/test/slotable_test.rb b/test/sandbox/test/slotable_test.rb index ce94eea0c..9f8961ed8 100644 --- a/test/sandbox/test/slotable_test.rb +++ b/test/sandbox/test/slotable_test.rb @@ -720,13 +720,24 @@ def test_slot_with_content_shorthand assert component.title.content? end - def test_raises_error_on_conflicting_slot_names_with_content - exeption = assert_raises ViewComponent::RedefinedExistingMethodError do + def test_raises_error_on_conflicting_slot_names_with_content_for_renders_one + exception = assert_raises ViewComponent::RedefinedExistingMethodError do Class.new(ViewComponent::Base) do renders_one :tag end end - - assert_includes exeption.message, "declares a slot named tag" + + assert_includes exception.message, "declares a slot named tag" + end + + def test_raises_error_on_conflicting_slot_names_with_content_for_renders_many + exception = assert_raises ViewComponent::RedefinedExistingMethodError do + Class.new(ViewComponent::Base) do + renders_many :tags + end + end + + assert_includes exception.message, "declares a slot named tag" end + end \ No newline at end of file From 0727fe666b76ec9d05137078af505c43f088945f Mon Sep 17 00:00:00 2001 From: Hugo Chantelauze Date: Tue, 7 Nov 2023 11:22:20 +0100 Subject: [PATCH 5/9] changelog --- docs/CHANGELOG.md | 4 ++++ docs/index.md | 1 + 2 files changed, 5 insertions(+) diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 65d856715..f6f9ee3e7 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -10,6 +10,10 @@ nav_order: 5 ## main +* Add an error in case of conflict after naming a slot with the name of an existing method. + + *Hugo Chantelauze* + * Document the capture compatibility patch on the Known issues page *Simon Fish* diff --git a/docs/index.md b/docs/index.md index a54f26bf8..d45b4a2e9 100644 --- a/docs/index.md +++ b/docs/index.md @@ -140,6 +140,7 @@ ViewComponent is built by over a hundred members of the community, including: fsateler fugufish g13ydson +hchtlz horacio horiaradu yykamei From 9116d517aa382c4daf20a54e0d8f873929febb1a Mon Sep 17 00:00:00 2001 From: Hugo Chantelauze <93914147+hchtlz@users.noreply.github.com> Date: Tue, 7 Nov 2023 11:27:59 +0100 Subject: [PATCH 6/9] Apply suggestions from code review Co-authored-by: Hans Lemuet --- docs/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 4d6ab6db0..d110e73b7 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -10,7 +10,7 @@ nav_order: 5 ## main -* Add the error `RedefinedExistingMethodError` in case of conflict after naming a slot with the name of an existing method. +* Raise the error `RedefinedExistingMethodError` when declaring a slot which conflicts with an existing method. *Hugo Chantelauze* From 312e6daa06acbd9a990e6dbe554b175c1c2a6f96 Mon Sep 17 00:00:00 2001 From: Hugo Chantelauze <93914147+hchtlz@users.noreply.github.com> Date: Tue, 7 Nov 2023 11:29:38 +0100 Subject: [PATCH 7/9] Apply suggestions from code review Co-authored-by: Hans Lemuet --- test/sandbox/test/slotable_test.rb | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/test/sandbox/test/slotable_test.rb b/test/sandbox/test/slotable_test.rb index 3c087df69..1961ff76b 100644 --- a/test/sandbox/test/slotable_test.rb +++ b/test/sandbox/test/slotable_test.rb @@ -719,7 +719,6 @@ def test_slot_with_content_shorthand assert component.title.content? end - def test_slot_names_cannot_start_with_call_ assert_raises ViewComponent::InvalidSlotNameError do Class.new(ViewComponent::Base) do @@ -742,7 +741,7 @@ def test_slot_names_can_start_with_call end end - def test_raises_error_on_conflicting_slot_names_with_content_for_renders_one + def test_raises_error_on_slot_name_conflicting_with_existing_method_for_renders_one exception = assert_raises ViewComponent::RedefinedExistingMethodError do Class.new(ViewComponent::Base) do renders_one :tag @@ -752,7 +751,7 @@ def test_raises_error_on_conflicting_slot_names_with_content_for_renders_one assert_includes exception.message, "declares a slot named tag" end - def test_raises_error_on_conflicting_slot_names_with_content_for_renders_many + def test_raises_error_on_slot_name_conflicting_with_existing_method_for_renders_many exception = assert_raises ViewComponent::RedefinedExistingMethodError do Class.new(ViewComponent::Base) do renders_many :tags @@ -761,5 +760,4 @@ def test_raises_error_on_conflicting_slot_names_with_content_for_renders_many assert_includes exception.message, "declares a slot named tag" end - end From e1b6fca56fe3504d61e9339ed3b1b3e721299473 Mon Sep 17 00:00:00 2001 From: Hugo Chantelauze Date: Tue, 7 Nov 2023 11:33:11 +0100 Subject: [PATCH 8/9] fix lint --- test/sandbox/test/slotable_test.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/sandbox/test/slotable_test.rb b/test/sandbox/test/slotable_test.rb index 1961ff76b..9735c4a31 100644 --- a/test/sandbox/test/slotable_test.rb +++ b/test/sandbox/test/slotable_test.rb @@ -747,17 +747,17 @@ def test_raises_error_on_slot_name_conflicting_with_existing_method_for_renders_ renders_one :tag end end - + assert_includes exception.message, "declares a slot named tag" end - + def test_raises_error_on_slot_name_conflicting_with_existing_method_for_renders_many exception = assert_raises ViewComponent::RedefinedExistingMethodError do Class.new(ViewComponent::Base) do renders_many :tags end end - + assert_includes exception.message, "declares a slot named tag" end -end +end \ No newline at end of file From 505d6542b8c56b865709c79e39de261d24a8befa Mon Sep 17 00:00:00 2001 From: Hugo Chantelauze Date: Tue, 7 Nov 2023 11:35:40 +0100 Subject: [PATCH 9/9] fix lint --- test/sandbox/test/slotable_test.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/sandbox/test/slotable_test.rb b/test/sandbox/test/slotable_test.rb index 9735c4a31..8938bf8d0 100644 --- a/test/sandbox/test/slotable_test.rb +++ b/test/sandbox/test/slotable_test.rb @@ -719,6 +719,7 @@ def test_slot_with_content_shorthand assert component.title.content? end + def test_slot_names_cannot_start_with_call_ assert_raises ViewComponent::InvalidSlotNameError do Class.new(ViewComponent::Base) do @@ -760,4 +761,4 @@ def test_raises_error_on_slot_name_conflicting_with_existing_method_for_renders_ assert_includes exception.message, "declares a slot named tag" end -end \ No newline at end of file +end