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

fix(templates): fixed string interpolation in template strings #571

Merged
merged 1 commit into from
Oct 20, 2024

Conversation

BirdeeHub
Copy link
Contributor

addresses #561

@BirdeeHub BirdeeHub force-pushed the interpolationSurprise branch from 3f5a27e to 1b4edcb Compare October 4, 2024 10:18
@BirdeeHub
Copy link
Contributor Author

Edit:

Actually, you still cant escape them...

You couldnt before, but, after this fix you still wont be able to.

Working on a better version now. Hopefully will have in a few days

@BirdeeHub
Copy link
Contributor Author

Ok, well, it escapes properly in all but 1 case now in my local copy that Im working on.

I know what that case is and why it happens, and I could disgustingly fix it but id rather like... nicely fix it.

`test \${x}`

currently this case fails still in my local copy.

But Im close. I just need to find a good way to fix the last case

@BirdeeHub
Copy link
Contributor Author

I did have to split up consumeIdentifier and consumeTemplateIdentifier into 2 separate functions though.

Actually, since the current one on here doesnt escape at ALL I will push the updated version in the meantime so you can see where its at.

@BirdeeHub BirdeeHub force-pushed the interpolationSurprise branch from 1b4edcb to 3e29b49 Compare October 4, 2024 15:05
@BirdeeHub BirdeeHub marked this pull request as draft October 4, 2024 15:07
@BirdeeHub
Copy link
Contributor Author

BirdeeHub commented Oct 4, 2024

When I fix the final escaping case:

`test \${x}`

This PR will be reinstated as no longer a draft.

@BirdeeHub
Copy link
Contributor Author

https://github.com/BirdeeHub/_hyperscript/blob/3e29b493e3eb032f5177371218b4f6cf18485c27/src/_hyperscript.js#L279-L286

^ this clause is the one catching that 1 case that I havent addressed yet...

@BirdeeHub BirdeeHub force-pushed the interpolationSurprise branch from 3e29b49 to 8e930ad Compare October 4, 2024 16:27
@BirdeeHub BirdeeHub marked this pull request as ready for review October 4, 2024 16:28
@BirdeeHub
Copy link
Contributor Author

It works now :)

@1cg
Copy link
Contributor

1cg commented Oct 20, 2024

Thank you for this change, did you run the entire test suite against it?

@1cg 1cg merged commit 7ffd186 into bigskysoftware:master Oct 20, 2024
4 checks passed
@BirdeeHub
Copy link
Contributor Author

BirdeeHub commented Oct 21, 2024

yeah I ran all the tests, I also tried it out in the playground.

I also added the new test which tests many different permutations of the interpolation. In the test I had to double escape because it gets passed through JS first, but in the playground it works as expected, you can escape with \${} and it will not interpolate

Also it seems your test suite fails one of the tests when ran via opening test/index.html file with a browser directly but succeeds if you serve with python or something and then go to the page in the browser? This is unrelated to the interpolation syntax though, I forget which but I think it was the socket one? I just remember the failure making sense when I looked at the code of the offending test.

@BirdeeHub BirdeeHub deleted the interpolationSurprise branch October 21, 2024 12:01
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

2 participants