-
Notifications
You must be signed in to change notification settings - Fork 21.5k
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 final newline from Action View renders #42279
Conversation
@boardfish Nice work! I think we probably want to add a changelog, but since we're defaulting to the current behavior I think this is a reasonable approach. |
🤔 I'm not sure but maybe looking at similar config options (and their origin) will give us a good idea of requirements.
I'm also not familiar enough here to make a firm decision, but @jhawthorn may be a good person to ask.
Here too.
This seems we would at least be providing a path forward for them to use in a future release? Correct me if I'm wrong but it seems there is nothing on Rails side to do here. |
RE documentation, I'll do some digging. I think the fact that the option is commented and in the config files is sufficient, but it wouldn't hurt to be sure.
I guess the real question is whether this config option should affect the output of |
I see that render mentions this method but I've never actually seen it used:
Could you show me an example? It looks like it's up to the Object which implements this method to define that behavior. |
Having had a look in the ViewComponent codebase, I don't feel we should make a change to |
Documentation for this should be added to the section on Configuring Action View in the Rails guides. I've already done this as part of this PR. Action View configuration options occasionally get a mention in the API documentation where relevant. I think this isn't necessary for this config option. |
I've realized a potential flaw in what I've got here - I'm not sure the option should be commented out in the environment-specific config. I imagine folks would typically set this option in |
@boardfish w/r/t where to put the config I'm not sure, but I think Going back to what you said about |
Sounds good. I'll make that change and then this ought to be ready for wider review. |
There we go! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I think you may need to squash your commits but that can also wait until after some closer review
@boardfish thank you for the thoughtful PR and description of the issue ✨.
I think I agree. It would be nice if we could avoid extraneous, unintentional whitespace by default. One question I have is whether there are cases we'd want to keep that whitespace. For example, at the end of the full document (ie. usually the end of the layout) we should probably have a newline to meet that same "text file" standard 😅. Another case might be when using, say,
Yeah. My biggest concern with this currently is where we call I feel like this might make more sense to implement as part of the ERB (Erubi) handler. I'd argue the "extra" newline is a detail of ERB's syntax (that its source forms the output), and trailing newlines from many other handlers ( |
Thanks for checking this out, @jhawthorn. I'll investigate dealing with this at the ERB level tomorrow. To answer some of your concerns:
Other template handlers might also require this change, but I'm not sure that should be within the remit of this PR. I can handle those in further PRs, which should be a lot easier if the solution for ERB can be defined in this one. It'd help me to know which tests to keep an eye on to make sure escaping is still safe. |
Okay, I've moved it to where I think makes sense, though I'm aware it doesn't have consideration for those edge cases at present. I was also curious about this:
Where were you able to do this? Hopefully with the change being where it is now, this shouldn't be a problem, but it'd be helpful to know how to check this out. Also, when it comes to squashing my commits, I suppose it'd be best to squash them into one, right? |
@boardfish Yeh generally you will be asked to do this once the PR is approved (perhaps verbally first). It sounds like the remaining question is how this affects the html-safety behavior? Should we just make it default and run the tests to verify that? Do we still have to worry about non-Erb templates, e.g. builder being an issue here? |
I've given this a shot just now. I didn't investigate the test output super thoroughly, but in all the cases I saw, failures only ever arose because a newline was part of the test expectation.
This code theoretically shouldn't interfere with any other templates now. If a similar change needs to be introduced for other template types, I think that can be done separately. |
Left one more suggestion (moving the config option) but I think with that and a squash this is good to go 🚀 😁 |
railties/lib/rails/generators/rails/app/templates/config/application.rb.tt
Outdated
Show resolved
Hide resolved
Add failing test for views with trailing newlines Add and test config option Move config option to config/application.rb Move implementation to ERB template handler Move config option to ActionView::Template::Handlers::ERB
64da20d
to
fe5ef42
Compare
All suggestions addressed, and I've rebased against I took the option out of the documentation and |
Awesome! Thanks so much @boardfish |
Summary
Closes #42201
Introduces the
strip_trailing_newlines
option toActionView::Template::Handlers::ERB
, which removes trailing newlines from rendered output. This means that partials can be rendered inline without introducing additional whitespace to the output, which affects how the browser may render it.Other Information
To save you recapping those issues, ending files with a newline is something of a standard, as explained by this StackOverflow thread's answers. We lint for this @raisedevs. Ordinarily, it doesn't introduce problems, but Rails renders trailing newlines, which are interpreted by the browser as whitespace. So when a partial is rendered inline with other text, the newlines cause a space between the rendered partial and the next character.
I think it should be the default that trailing newlines are stripped, and I'm not sure it'd be harmful to introduce that. But I've kept
false
as the default because that's how things are now.Some questions I had while writing this
👋 This is my first contribution to Rails, so I've tried to be meticulous about it. But there are a few questions I have around this:
I imagine the config option might have a place in the documentation, so I'll look to update this if that's correct.Added to the Guides.ActionView::Template::Handlers
.render_in
, it might be possible to fix that, but whether that's even Action View's responsibility is another story entirely.