Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mixing yield call styles #54

Merged
merged 5 commits into from
Sep 5, 2022
Merged

Conversation

seanpdoyle
Copy link
Collaborator

@seanpdoyle seanpdoyle commented Aug 25, 2022

Mixing yield call styles

Follow-up to #41
Follow-up to #51

While the changes introduced in #41 work in theory and pass the tests
written for the alias, they fail to work in practice.

Whenever a Partial#yield call without arguments is made in the same
partial as a Partial#yield call with arguments, the content of the
Partial#yield call made with a name is ignored.

Likewise, when a Partial#yield call is made with an object argument,
without a name, it raises an exception:

Error:
RendererTest#test_mixing_Partial#yield_call_styles_with_objects_renders_all_captured_content:
ActionView::Template::Error: wrong number of arguments (given 1, expected 0)
    [REDACTED]/lib/ruby/gems/3.1.0/gems/activesupport-7.0.3/lib/active_support/core_ext/object/blank.rb:25:in
`present?'
    [REDACTED]/lib/ruby/gems/3.1.0/gems/activesupport-7.0.3/lib/active_support/option_merger.rb:28:in `method_
missing'
    [REDACTED]/bullet-train-co/nice_partials/lib/nice_partials/partial/section.rb:37:in `concat'
    [REDACTED]/bullet-train-co/nice_partials/lib/nice_partials/partial/section.rb:24:in `write_content_for'
    [REDACTED]/bullet-train-co/nice_partials/lib/nice_partials/partial/section.rb:8:in `content_for'
    [REDACTED]/bullet-train-co/nice_partials/lib/nice_partials/partial.rb:46:in `content_for'
    [REDACTED]/bullet-train-co/nice_partials/lib/nice_partials/partial.rb:27:in `yield'
    [REDACTED]/bullet-train-co/nice_partials/test/fixtures/yields/_mixed_with_object.html.erb:2:in `___[REDACTED]_bullet_
train_co_nice_partials_test_fixtures_yields__mixed_with_object_html_erb___1507586996035634049_1460'

This commit adds two failing tests to the suite.

To help troubleshoot this behavior (and to introduce it to the test
harness!), this commit adds a dependency on Capybara, and includes the
Capybara::Minitest::Assertions module into NicePartials::Test.

Ignore . when scanning templates for yield

First, the issue between Partial#output_buffer and Partial#yield is
rooted in the NicePartials::PartialRendering#has_capturing_yield?
check's \byield[\(? ]+(%>|[^:]) regular expression.

The "working" partial passes the regular
expression
, which does not detect
a bare yield call:

<div id="mixed_yield">
  <%= partial.yield :slot %>
  <%= partial.output_buffer %>
</div>

A partial with a bare yield call does not pass the regular
expression
, since it detects the
bare yield call:

<div id="mixed_yield">
  <%= partial.yield :slot %>
  <%= yield %>
</div>

Unfortunately, a partial with partial.yield also does not pass the
regular expression
:

<div id="mixed_yield">
  <%= partial.yield :slot %>
  <%= partial.yield %>
</div>

The \b word boundary in the regular expression isn't treating
partial.yield any differently than yield. Replacing the \b with
/[^\.\b] seems to do the trick.

@seanpdoyle
Copy link
Collaborator Author

I'm not sure how to best troubleshoot this issue, so this is part bug report, part proposal.

Follow-up to bullet-train-co#41
Follow-up to bullet-train-co#51

While the changes introduced in bullet-train-co#41 work in theory and pass the tests
written for the alias, they fail to work in practice.

Whenever a `Partial#yield` call _without_ arguments is made in the same
partial as a `Partial#yield` call _with_ arguments, the content of the
`Partial#yield` call made with a name is ignored.

Likewise, when a `Partial#yield` call is made with an object argument,
without a name, it raises an exception:

```
Error:
RendererTest#test_mixing_Partial#yield_call_styles_with_objects_renders_all_captured_content:
ActionView::Template::Error: wrong number of arguments (given 1, expected 0)
    [REDACTED]/lib/ruby/gems/3.1.0/gems/activesupport-7.0.3/lib/active_support/core_ext/object/blank.rb:25:in
`present?'
    [REDACTED]/lib/ruby/gems/3.1.0/gems/activesupport-7.0.3/lib/active_support/option_merger.rb:28:in `method_
missing'
    [REDACTED]/bullet-train-co/nice_partials/lib/nice_partials/partial/section.rb:37:in `concat'
    [REDACTED]/bullet-train-co/nice_partials/lib/nice_partials/partial/section.rb:24:in `write_content_for'
    [REDACTED]/bullet-train-co/nice_partials/lib/nice_partials/partial/section.rb:8:in `content_for'
    [REDACTED]/bullet-train-co/nice_partials/lib/nice_partials/partial.rb:46:in `content_for'
    [REDACTED]/bullet-train-co/nice_partials/lib/nice_partials/partial.rb:27:in `yield'
    [REDACTED]/bullet-train-co/nice_partials/test/fixtures/yields/_mixed_with_object.html.erb:2:in `___[REDACTED]_bullet_
train_co_nice_partials_test_fixtures_yields__mixed_with_object_html_erb___1507586996035634049_1460'
```

This commit adds two failing tests to the suite.

To help troubleshoot this behavior (and to introduce it to the test
harness!), this commit adds a dependency on Capybara, and includes the
[Capybara::Minitest::Assertions][] module into `NicePartials::Test`.

[Capybara::Minitest::Assertions]: https://rubydoc.info/github/teamcapybara/capybara/master/Capybara/Minitest/Assertions
@kaspth
Copy link
Contributor

kaspth commented Aug 25, 2022

Interesting — yeah, this probably needs some thought. Though it also raises my concerns about furthering Rails' multiple use cases of yield in Nice Partials. @seanpdoyle since you've looked into this, would an alternative name to output_buffer remedy this as well?

First, the issue between `Partial#output_buffer` and `Partial#yield` is
rooted in the `NicePartials::PartialRendering#has_capturing_yield?`
check's `\byield[\(? ]+(%>|[^:])` regular expression.

The "working" partial [passes the regular
expression](https://rubular.com/r/qH0ZY8hY33LPEB), which does not detect
a bare `yield` call:

```erb
<div id="mixed_yield">
  <%= partial.yield :slot %>
  <%= partial.output_buffer %>
</div>
```

A partial with a bare `yield` call [does not pass the regular
expression](https://rubular.com/r/DwK3LcyrQtRw2m), since it detects the
bare `yield` call:

```erb
<div id="mixed_yield">
  <%= partial.yield :slot %>
  <%= yield %>
</div>
```

Unfortunately, a partial with `partial.yield` [also does not pass the
regular expression](https://rubular.com/r/gJ4Qu7z7eBBAq4):

```erb
<div id="mixed_yield">
  <%= partial.yield :slot %>
  <%= partial.yield %>
</div>
```

The `\b` word boundary in the regular expression isn't treating
`partial.yield` any differently than `yield`. Replacing the `\b` with
`/[^\.\b]` seems to do the trick.
When writing content, only concatenate "argument" content when its a
descendant of [String][] (this includes instances of
[ActiveSupport::SafeBuffer][] created by calls to `#html_safe`).

If content is `nil` or an object, capture content from the `&block`.

The conditional guard **intentionally** compares `#class` and `String`
with the [<=][] operator instead of the [Object#is_a?][] predicate so
that object instances created by [with_options][] can pass through the
conditional without invoking `is_a?` with the incorrect number of
arguments.

[String]: https://ruby-doc.org/core/String.html
[ActiveSupport::SafeBuffer]: https://edgeapi.rubyonrails.org/classes/ActiveSupport/SafeBuffer.html
[<=]: https://ruby-doc.org/core-3.1.2/Module.html#method-i-3C-3D
[Object#is_a?]: https://ruby-doc.org/core-3.1.2/Object.html#method-i-is_a-3F
[with_options]: https://edgeapi.rubyonrails.org/classes/Object.html#method-i-with_options
@seanpdoyle seanpdoyle marked this pull request as ready for review August 26, 2022 00:33
Copy link
Contributor

@kaspth kaspth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had no idea that \b in the regex wouldn't be enough.

Glad we figured this out ❤️

@kaspth kaspth added this to the v1.0.0 milestone Aug 29, 2022
@kaspth
Copy link
Contributor

kaspth commented Sep 5, 2022

I've noted some issues since #57, but I'll merge and fix those on main.

@kaspth kaspth merged commit a985f7b into bullet-train-co:main Sep 5, 2022
kaspth added a commit that referenced this pull request Sep 5, 2022
@kaspth
Copy link
Contributor

kaspth commented Sep 5, 2022

Done in e1a5c7f

kaspth added a commit that referenced this pull request Sep 6, 2022
* main:
  Ignore /tmp if you're running BT in tmp/starter like CI does
  Fix some CHANGELOG issues and refit some examples + words
  Skip needless CI steps (#60)
  We don't need to depend on sqlite3 that was for an earlier version of the ViewComponent integration
  Fix #54 to use new section yield syntax
  Mixing `yield` call styles (#54)
  Expose Sections as an alternative to `content_for` (#57)
  Introduce Capybara to the test suite (#55)
  Add `content_from` to let partials relay contents (#53)
  v0.1.9
  v0.1.8
  Document release 🎉 (#52)
  Rename `Partial#output_buffer` to `Partial#yield` (#41)
  Allow outer partial access when capturing (#49)
  Fix accessing `partial` before rendering leaks state (#47)
  Use load hooks for monkey_patch loading (#48)
kaspth added a commit that referenced this pull request Dec 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants