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

Fix multi-line strings bug #20

Open joshuakb2 opened 4 years ago

joshuakb2 commented 4 years ago

Fix bug #16

This:

main =
    <div>Hi</div>

should become

main =
    Html.node "div" [] [Html.text "Hi"]

and this:

main =
    <p>
        foo
    </p>

should become

main =
    Html.node "p" [] [Html.text """
        foo
    """]
pzavolinsky commented 4 years ago

Hey @joshuakb2 thanks a lot for this contrib!

I left you two comments in the PR above, one of which I believe should fix the broken test.

Could you also add more tests for mixed content (e.g. a multiline node with other multiline inside)?

Let me know when I can review again (also please squash all commits down to a single one).

Thanks again, Cheers!

joshuakb2 commented 4 years ago

I'd be happy to make those changes, but I'm not sure which test is failing.

Here's what I'm doing to build -- have I missed something?

joshua@Joshua-X1:~/projects/elmx$ npm ci
added 495 packages in 3.776s
joshua@Joshua-X1:~/projects/elmx$ ./node_modules/.bin/gulp 
[14:08:15] Using gulpfile ~/projects/elmx/gulpfile.js
[14:08:15] Starting 'clean'...
[14:08:15] Starting 'copy-bundle'...
[14:08:15] Finished 'copy-bundle' after 4.1 ms
[14:08:15] Finished 'clean' after 20 ms
[14:08:15] Starting 'build'...
[14:08:16] Finished 'build' after 1.21 s
[14:08:16] Starting 'test'...
......................................................................................................

49 scenarios (49 passed)
102 steps (102 passed)
0m00.011s
[14:08:17] Finished 'test' after 451 ms
[14:08:17] Starting 'bundle'...
[14:08:17] Finished 'bundle' after 392 ms
[14:08:17] Starting 'uglify'...
[14:08:17] Finished 'uglify' after 843 μs
[14:08:17] Starting 'default'...
[14:08:17] Finished 'default' after 13 μs
joshua@Joshua-X1:~/projects/elmx$
joshuakb2 commented 4 years ago

Oh, I'm sorry, you're referring to the one that I @ignored. The issue with that one actually has nothing to do with the triple quotes. The generator doesn't seem to output any whitespace output in that case, before, after, or between the two text variables.

joshuakb2 commented 4 years ago

I made the first change you suggested, and I added a comment to full.feature to clarify some confusing syntax. I also squashed everything into one commit.

The test scenario I added with an @ignore tag doesn't have anything to do with the multi-line bug -- I can reproduce the problem without multi-line input. However, I took a look at how Babel handles whitespace in JSX, and it looks like any time there is a new line in a sequence of whitespace characters, all of the whitespace is omitted (or replaced with a single space if the whitespace occurs between two bits of visible text).

This difference in behavior leads me to think that we shouldn't be outputting multi-line Elm syntax at all. I think implementing this differently might also lead to a better solution to the failing test case I added.

Feel free to review what I've done so far, but I'm going to continue working on this in the meantime.

pzavolinsky commented 4 years ago

Hey @joshuakb2 thanks a lot for your time and your work.

Let me know how the research goes.

(It was a great idea, IMO, to see how other similar conversions work (e.g. HTML, JSX, etc.))

Cheers!