Skip to content

properly support nested ERB for .jst.hamlc with Sprockets 4#193

Merged
mathieujobin merged 1 commit intoemilioforrer:masterfrom
teamsimplepay:erb_support_for_jst_sprockets4
Dec 2, 2024
Merged

properly support nested ERB for .jst.hamlc with Sprockets 4#193
mathieujobin merged 1 commit intoemilioforrer:masterfrom
teamsimplepay:erb_support_for_jst_sprockets4

Conversation

@ashkulz
Copy link
Copy Markdown
Contributor

@ashkulz ashkulz commented Jul 22, 2024

While we don't really use this, there's a logical bug here which will cause an issue if nested ERB templates are used with .jst.hamlc files.

@voducvu can you also test this and verify that it works before @mathieujobin approves/merges this?

@mathieujobin
Copy link
Copy Markdown
Collaborator

I don't have a clear understand of what problem it fixes and how many people would benefit from the change.

is it possible to add some tests that would demonstrate the problem and be solved by this change?

@ashkulz
Copy link
Copy Markdown
Contributor Author

ashkulz commented Nov 18, 2024

@mathieujobin we don't use this feature, but logically it's similar to #192 -- you want to associate a file extension with a single MIME type and register the right transformers to get it to the right MIME type which we want as the output.

You can see that the same MIME type is being used for .jst.hamlc.erb which is wrong. I'm not sure I can come up with a test for it quickly, though 😅

@mathieujobin mathieujobin merged commit ed011f0 into emilioforrer:master Dec 2, 2024
@voducvu
Copy link
Copy Markdown
Contributor

voducvu commented Dec 4, 2024

While we don't really use this, there's a logical bug here which will cause an issue if nested ERB templates are used with .jst.hamlc files.

@voducvu can you also test this and verify that it works before @mathieujobin approves/merges this?

hi @ashkulz @mathieujobin, I'm sorry for the late response due to my personal issues.
The problem this PR solves slighly the same as #192. It registers both file extension with and without .jst as one mimetype which will be processed the same. While it must be processed differently. Therefore, the fix approach is the same as #192, register different mimetype and processor for these 2 extensions.

The PR LGTM though

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