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

Add test coverage for use of turbo_stream helpers in components #1227

Merged
merged 14 commits into from
Aug 20, 2024

Conversation

boardfish
Copy link
Collaborator

Summary

Replicates #1137 with the aim of supporting #1106.

Other Information

I'm not sure how to get this test to pass. I've tried using helpers.tag.turbo_stream and helpers.tag.template too, but neither seems to change the output.

Desired output (whitespace formatted for readability):

<turbo-stream action="update" target="area1">
    <template>
        <span>Hello, world!</span>
    </template>
</turbo-stream>

Actual output (whitespace formatted for readability):

<span>Hello, world!</span>
<turbo-stream action="update" target="area1">
    <template>
        &lt;span&gt;Hello, world!&lt;/span&gt;
    </template>
</turbo-stream>

Regardless of whether helpers.tag.turbo_stream or turbo_stream.update is used, what seems to be happening is that the block that is received is concatenated to the view as normal, then done again without being marked as html_safe within the tags.

Implementation of the turbo_stream tag builder starts around here.This is the object returned by calling turbo_stream against the view.

@boardfish
Copy link
Collaborator Author

Turbo doesn't support Rails < 6 - that seems to come right down to installation, since the builds are failing on dependencies. Was Appraisal ever added to our testing setup? I seem to remember seeing talk about it in another PR. I think that'd resolve this issue.

@Spone
Copy link
Collaborator

Spone commented Jan 3, 2022

Turbo doesn't support Rails < 6 - that seems to come right down to installation, since the builds are failing on dependencies. Was Appraisal ever added to our testing setup? I seem to remember seeing talk about it in another PR. I think that'd resolve this issue.

Yes this is still a work in progress here: #1225

Feel free to review :)

@Spone Spone mentioned this pull request Jan 4, 2022
@Spone
Copy link
Collaborator

Spone commented Jan 5, 2022

@boardfish #1225 is merged, you can rebase and add turbo as a conditional dependency!

@boardfish
Copy link
Collaborator Author

@Spone I've tried to move things over to using Appraisal, but CI seems to be failing because it doesn't recognize turbo-rails as having been installed. I'm not having this problem locally when I run bundle exec appraisal install && bundle exec appraisal rake on a version that's compatible with turbo-rails (Ruby 3, rails-head). It doesn't look to me as though the tests are running via appraisal in CI, but I'm not confident around the source of the issue - would you be able to look into it?

@Spone
Copy link
Collaborator

Spone commented Feb 14, 2022

It doesn't look to me as though the tests are running via appraisal in CI, but I'm not confident around the source of the issue - would you be able to look into it?

I guess we will have to wait for #1230 (see this comment)

@ViewComponent ViewComponent deleted a comment from primer-css May 23, 2022
@Spone
Copy link
Collaborator

Spone commented Feb 6, 2023

We're now waiting for #1308 :)

@reeganviljoen
Copy link
Collaborator

@Spone I see #1308 has been merged, are you still blocked on this ?

@boardfish
Copy link
Collaborator Author

Ooh, thanks for the reminder about this one @reeganviljoen – in theory we're not blocked on it anymore, but it's been a long time since I last looked at it 😅

@joelhawksley
Copy link
Member

@Spone @boardfish @reeganviljoen any interest in taking this on, or should I close?

@boardfish
Copy link
Collaborator Author

I'll rebase and see where that lands us – hopefully we're able to get it merged 🤞

@reeganviljoen
Copy link
Collaborator

@joelhawksley i had to get investge this the other day for work reasons

Tldr the tag helpers use concat wich trips up the capture

@boardfish
Copy link
Collaborator Author

Looks like we're subject to hotwired/turbo-rails#74 here. That's perhaps the final blocker – the purist in me thinks it's not worth bringing Action Cable in just to get this over the line, and that the capture compatibility patch ought to fix this. But equally I'm thinking we shouldn't be encouraging folks towards that if they can avoid it.

Also, while it's been a hot minute since I checked #1106, fixing this between ourselves and the turbo-rails folks would really help these two libraries to work together. They're both pretty core to the Rails experience now too, so the easier we can make it, the better.

@joelhawksley
Copy link
Member

@boardfish I'm fine adding ActionCable to our test stack as it's present in a lot of Rails apps ❤️

@boardfish
Copy link
Collaborator Author

Awesome, I'll do that next – I expect that to get this passing. I might see if I can contribute to turbo-rails to get this fully resolved afterwards.

@boardfish boardfish removed the blocked label Aug 17, 2024
@boardfish
Copy link
Collaborator Author

Alright, all set! We now require Action Cable in the sandbox app so that the Turbo Stream tag helper works. I also found that because of the age of this PR there was still some support for Rails versions we don't test against kicking around, so I got rid of that.

@rmacklin rmacklin changed the title Add failing test for rendering turbo stream tags Add support for rendering turbo stream tags Aug 17, 2024
docs/CHANGELOG.md Outdated Show resolved Hide resolved
@joelhawksley joelhawksley merged commit 5d0e075 into main Aug 20, 2024
19 checks passed
@joelhawksley joelhawksley deleted the test-turbo-stream-rendering branch August 20, 2024 20:07
@boardfish boardfish changed the title Add support for rendering turbo stream tags Add test coverage for use of turbo_stream helpers in components Aug 26, 2024
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.

4 participants