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

Strip trailing whitespace in templates #1379

Merged
merged 13 commits into from
Jun 21, 2022
Merged

Conversation

camertron
Copy link
Contributor

@camertron camertron commented Jun 2, 2022

What are you trying to accomplish?

Today a co-worker raised an issue with the LinkComponent in primer/view_components where it appeared the component was adding a newline to the end of the rendered output. It turned out there was a newline at the end of the corresponding template, which the ERB compiler faithfully turned into @output_buffer.append("\n"). Most of the time this isn't a problem, but it's common to include LinkComponent in a sentence. If it comes at the end of the sentence, there will be a space between the link text and the period.

IMHO this is pretty surprising behavior considering that the *nix standard is to leave a single trailing newline in all files. In almost all cases I don't think anyone will notice if view_component were to remove trailing whitespace, but I'm curious to hear from others.

What approach did you choose and why?

I added an .rstrip call in the compiler that trims trailing whitespace from the template before compiling it.

@camertron camertron requested a review from a team as a code owner June 2, 2022 20:10
@Spone
Copy link
Collaborator

Spone commented Jun 2, 2022

Wasn't this solved by #913 and rails/rails#42279? 🤔

@camertron
Copy link
Contributor Author

@Spone If I'm understanding the VC and Rails PRs, then I think this is a separate problem. In this case, stripping newlines from the compiled ERB won't help because the ERB contains @output_buffer.append("\n") instead of a literal trailing \n character. The template itself has to be stripped before compilation to avoid that extra append call.

@Spone
Copy link
Collaborator

Spone commented Jun 2, 2022

@camertron, makes sense, thanks for clarifying!

Copy link
Contributor

@BlakeWilliams BlakeWilliams left a comment

Choose a reason for hiding this comment

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

LGTM!

I lack context, but is there any risk of this breaking existing templates/behavior that would warrant a config option + deprecation warning?

@camertron camertron requested a review from Spone June 3, 2022 17:49
@camertron
Copy link
Contributor Author

@BlakeWilliams hmm that's a really good call re: email templates. Maybe it makes sense to put the config option on BaseComponent so you can set it on a per-component basis.

@joelhawksley
Copy link
Member

@BlakeWilliams hmm that's a really good call re: email templates. Maybe it makes sense to put the config option on BaseComponent so you can set it on a per-component basis.

Hmm. I'd love to avoid a config option long-term. Is there any way we can test this hypothesis?

Copy link
Member

@joelhawksley joelhawksley left a comment

Choose a reason for hiding this comment

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

Needs docs beyond API

camertron and others added 3 commits June 16, 2022 10:15
Use a default value for the boolean flag so you can simply call `strip_trailing_newlines` in your component.

Co-authored-by: Joel Hawksley <joel@hawksley.org>
@camertron camertron mentioned this pull request Jun 16, 2022
23 tasks
@camertron camertron changed the title Strip trailing newlines in templates Strip trailing whitespace in templates Jun 16, 2022
@camertron
Copy link
Contributor Author

camertron commented Jun 16, 2022

We discussed the config option internally and came to consensus that a per-component class method is ok 👍 eg.

class MyComponent < ViewComponent::Base
  strip_trailing_whitespace
end

docs/api.md Outdated Show resolved Hide resolved
docs/known_issues.md Outdated Show resolved Hide resolved
test/sandbox/test/rendering_test.rb Show resolved Hide resolved
@joelhawksley joelhawksley merged commit 361c890 into main Jun 21, 2022
@joelhawksley joelhawksley deleted the strip_trailing_newlines branch June 21, 2022 13:14
claudiob pushed a commit to claudiob/view_component that referenced this pull request Dec 22, 2023
* Strip trailing newlines in templates

* Use rstrip so only trailing whitespace is trimmed

* Add changelog entry

* Add test demonstrating trailing newline behavior

* Use config option

* Fix linting issues

* Update lib/view_component/base.rb

Use a default value for the boolean flag so you can simply call `strip_trailing_newlines` in your component.

Co-authored-by: Joel Hawksley <joel@hawksley.org>

* Add additional docs; rename to strip_trailing_whitespace

* Fence code blocks in base.rb

* Move trailing whitespace docs to templates docs

Co-authored-by: Joel Hawksley <joel@hawksley.org>
claudiob pushed a commit to claudiob/view_component that referenced this pull request Jan 3, 2024
* Strip trailing newlines in templates

* Use rstrip so only trailing whitespace is trimmed

* Add changelog entry

* Add test demonstrating trailing newline behavior

* Use config option

* Fix linting issues

* Update lib/view_component/base.rb

Use a default value for the boolean flag so you can simply call `strip_trailing_newlines` in your component.

Co-authored-by: Joel Hawksley <joel@hawksley.org>

* Add additional docs; rename to strip_trailing_whitespace

* Fence code blocks in base.rb

* Move trailing whitespace docs to templates docs

Co-authored-by: Joel Hawksley <joel@hawksley.org>
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

4 participants