search

pzavolinsky / elmx

A tiny precompiler that takes an Elm program with embedded HTML and desugars the HTML into elm-html syntax. Elmx is to Elm what React's JSX is to Javascript
MIT License
351 stars 11 forks source link

Elmx produces invalid multiline strings #16

Open danneu opened 7 years ago

danneu commented 7 years ago 
<p>
    foo
</p>

produces

Html.node "p" [] [Html.text "
    foo
"]

(error: unexpected \n)

of course, that could be fixed with

Html.node "p" [] [Html.text """
    foo
"""]

for some reason it seems like elm switched from single-pair to triple quote multiline strings in the last year, and i can't find a changelog. maybe i'm going crazy.

skosch commented 6 years ago

This effectively breaks elmx :( – please merge @jakub-zawislak's PR!

joshuakb2 commented 4 years ago

Dang, it's been a year and a half. Is there any reason not to merge jakub-zawislak's PR @pzavolinsky? I'd like to use elmx in my project.

I guess I can just clone this repo and merge the PR myself rather than getting the dependency from npm.

pzavolinsky commented 4 years ago

Hi @joshuakb2 sorry this flew under my radar. As I explained here I'm not actively using ELM and I doubt I can maintain this package on my own.

When I initially reviewed the PR you mention (long ago), the changes, although pretty minimal, broke some tests (hinting at a possible bug, e.g. breaking everything but multi-line strings).

Again, since I'm not an ELM user anymore, I'm not sure if this is the expected behavior nowadays, but my gut suggest it's not.

In any case, the issue reported by @danneu is surely a bug, and if anyone wants to help with the code, either by building up from this PR or by starting from scratch, I'd be glad to review and merge.

Sorry it took me so long to reply, I'm currently focusing in other projects and it's hard for me to keep up with issues in this one (as you obviously know already).

I apologize for not being on top of this, and I kindly ask anyone interested in moving this project forward to help by either submitting PRs and/or reviewing them.

Cheers!

joshuakb2 commented 4 years ago

Hi @pzavolinsky,

Sorry, I didn't think to look at the PR until after commenting, where I noticed that you mentioned the failed tests.

Jakub's PR fixes the bug by simply using triple quotes for all string literals, regardless of whether or not they contain newlines. The tests simply failed because they were not updated to expect triple-quote string literals.

I cloned his PR and fixed the tests, so I can submit a new PR if you think Jakub's solution is well-conceived. Otherwise, do you think we should continue to use single quotes when strings do not contain any newlines?

Thanks for the quick response!

pzavolinsky commented 4 years ago

I cloned his PR and fixed the tests, so I can submit a new PR if you think Jakub's solution is well-conceived.

Awesome!

...do you think we should continue to use single quotes when strings do not contain any newlines?

Honestly I don't feel qualified to answer this. I guess it depends on what ELM specifies and I'm super rusty on ELM. If we could get some consensus out there on what should be done, I'll listen to what the ELM community thinks best. I know is asking too much, but if you could check some docs or other ELM projects to see what should be done that would be great.

Cheers!

joshuakb2 commented 4 years ago

I had trouble finding any guidance online, but in general I think it's better to use standard string notation when a string doesn't need multiline notation (for better readability), and luckily, implementing that behavior turned out to be very easy.

PR incoming.