Strip final newline from Action View renders

[boardfish]

Summary

Closes #42201

Introduces the strip_trailing_newlines option to ActionView::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.
• I'm not 100% confident that this is the best implementation. It achieves what I want it to, but I don't know if it's better placed in ActionView::Template::Handlers.
• I'm also not sure if my test is in the right part of the suite.
• As mentioned back over on the issue, this doesn't also resolve Final newline is rendered ViewComponent/view_component#913. Assuming that library still uses render_in, it might be possible to fix that, but whether that's even Action View's responsibility is another story entirely.

Thanks in advance for checking this out, folks.

[zzak]

@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.

[zzak]

I imagine the config option might have a place in the documentation, so I'll look to update this if that's correct.

🤔 I'm not sure but maybe looking at similar config options (and their origin) will give us a good idea of requirements.

I'm not 100% confident that this is the best implementation. It achieves what I want it to, but I don't know if it's better placed in ActionView::Template::Handlers.

I'm also not familiar enough here to make a firm decision, but @jhawthorn may be a good person to ask.

I'm also not sure if my test is in the right part of the suite.

Here too.

As mentioned back over on the issue, this doesn't also resolve ViewComponent/view_component#913. Assuming that library still uses render_in, it might be possible to fix that, but whether that's even Action View's responsibility is another story entirely.

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.

[boardfish]

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.

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.

I guess the real question is whether this config option should affect the output of render_in too, or whether that should be the responsibility of the object that implements it.

[zzak]

I guess the real question is whether this config option should affect the output of render_in too, or whether that should be the responsibility of the object that implements it.

I see that render mentions this method but I've never actually seen it used:

If an object responding to render_in is passed, render_in is called on the object, passing in the current view context.
Otherwise, a partial is rendered using the second parameter as the locals hash.

Could you show me an example? It looks like it's up to the Object which implements this method to define that behavior.

[boardfish]

render_in is quite new. I seem to remember it was merged in to Rails to help enable ViewComponent to work more seamlessly.

Having had a look in the ViewComponent codebase, render_in seems to be what outputs the compiled template to a string. Its job is to use the view context (if necessary) and output something as a string so that Action View can add it to the view. render_in can output a HTML-safe string.

I don't feel we should make a change to render_in itself to strip trailing newlines - it's the responsibility of the object to handle that output. But if we strip trailing newlines before the result of render_in is added to the template, then that's very clearly in Action View territory and it's more justifiable for the config option to control that output. I'm still on the fence as to whether we should do that at all, though — should it be the responsibility of the object with render_in to strip its own trailing newlines?

[boardfish]

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.

[boardfish]

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 config/application.rb.

[zzak]

@boardfish w/r/t where to put the config I'm not sure, but I think config/app makes sense (you probably won't be changing render behavior between environments).

Going back to what you said about render_in, I think it makes sense to assume the object defining this method will decide what to do with newlines.

[boardfish]

Sounds good. I'll make that change and then this ought to be ready for wider review.

[boardfish]

There we go!

[zzak]

LGTM, I think you may need to squash your commits but that can also wait until after some closer review

[jhawthorn]

@boardfish thank you for the thoughtful PR and description of the issue ✨.

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.

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, render collection: , newlines between each partial render would be more readable (but I'm not 100% convinced either way on this one).

I'm not 100% confident that this is the best implementation. It achieves what I want it to, but I don't know if it's better placed in ActionView::Template::Handlers.

Yeah. My biggest concern with this currently is where we call @body.chomp, body isn't a string, it's an OutputBuffer. Calling chomp will remove its html-safety (if you turn this on by default you should see many tests fail on escaping). We should also not expect that method to be available on the output buffer (it isn't if streaming) and we shouldn't try to modify it.

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 (.builder, .ruby) are intentional. It might be possible to call chomp on the ERB source... (but I haven't tested 😅)?

[boardfish]

Thanks for checking this out, @jhawthorn. I'll investigate dealing with this at the ERB level tomorrow. To answer some of your concerns:

• I think a newline after each partial in a render collection: might be reasonable, if only because there are alternatives to setting display: inline on each element, such as rendering inside a flex container.
• I'm less confident about a newline at the end of a HTTP response, but it's worth investigation.

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.

[boardfish]

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:

Calling chomp will remove its html-safety (if you turn this on by default you should see many tests fail on escaping).

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?

[zzak]

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?

[boardfish]

Should we just make it default and run the tests to verify that?

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.

Do we still have to worry about non-ERB templates, e.g. builder being an issue here?

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.

[jhawthorn]

Left one more suggestion (moving the config option) but I think with that and a squash this is good to go 🚀 😁

[boardfish]

All suggestions addressed, and I've rebased against main too.

I took the option out of the documentation and application.rb because now that this is isolated to ERB, it might not be relevant to all cases. If there's a need for it in other template handlers, the config option can be moved back into ActionView::Base and more widely documented.

[jhawthorn]

Awesome! Thanks so much @boardfish