[CS2] Support for CSX - equivalent of JSX #4551
Conversation
|
This is very impressive. Thanks for making this effort! If I could try to head off some expected critiques, if readers have an objection to whether JSX should be supported in the CoffeeScript ecosystem at all, please discuss that in #4529. For the purposes of this PR let’s please assume that CoffeeScript should support JSX, and let’s keep this thread to the specifics of this implementation. Reading through what you’ve done, it seems obvious that the approach I had in mind in #4529, of trying to treat A few general questions before getting into specifics:
|
GeoffreyBooth
changed the title
lib/coffeescript/parser.js
Outdated
| @@ -1,4 +1,4 @@ | |||
| /* parser generated by jison 0.4.17 */ | |||
| /* parser generated by jison 0.4.13 */ | |||
There was a problem hiding this comment.
Somehow you don’t have the latest version of Jison installed. Please npm install.
src/lexer.coffee
Outdated
| @@ -48,6 +48,7 @@ exports.Lexer = class Lexer | |||
| @seenExport = no # Used to recognize EXPORT FROM? AS? tokens. | |||
| @importSpecifierList = no # Used to identify when in an IMPORT {...} FROM? ... | |||
| @exportSpecifierList = no # Used to identify when in an EXPORT {...} FROM? ... | |||
| @includesCSX = no # Used to optimize CSX checks | |||
There was a problem hiding this comment.
In general, all comments should be complete sentences and end in periods. This makes the annotated source feel, well, annotated, like there is normal written commentary accompanying the code.
There was a problem hiding this comment.
The two lines above need fixing then .)
There was a problem hiding this comment.
Yes they do. They should also get some backticks, as they’re treated as Markdown when getting converted into the annotated source. I’ve generally been fixing comments as I come across them, rather than taking the time to go through all the code from top to bottom, so there are plenty of bad examples waiting to be fixed. All I’m asking you is to not add to the list of comments to correct 😄
test/csx.coffee
Outdated
| @@ -0,0 +1,616 @@ | |||
| # We usually do not check the actual JS output from the compiler, but since | |||
| # CSX is not readily supported by Node, we do it in this case | |||
| eqCSX = (cs, js) -> | |||
There was a problem hiding this comment.
There’s already a toJS function in global scope. Can we please just use it, and follow the pattern of the modules tests?
There was a problem hiding this comment.
@GeoffreyBooth is that boilerplate useful? I find the tests easier to read without it.
There was a problem hiding this comment.
Just keeps things DRY, and ultimately we’re trying to keep the codebase internally consistent. Not everything in there is done the way I’d prefer either.
| unquote: (literal) -> | ||
| unquoted = @value[1...-1] | ||
| if literal | ||
| unquoted.replace /\\n/g, '\n' |
There was a problem hiding this comment.
Could you explain what's special about the newlines here?
There was a problem hiding this comment.
See the (now outdated) comment below.
src/nodes.coffee
Outdated
| unquoted = @value[1...-1] | ||
| if literal | ||
| unquoted.replace /\\n/g, '\n' | ||
| .replace /\\"/g, '"' |
There was a problem hiding this comment.
Dealing with backslashes is never this simple. What if the backslash is already escaped? \\"
There was a problem hiding this comment.
We format strings to JS strings, which means escaping new lines and quotes. This function removes it, so the string's content is printed out literally.
"b\\"a" throws syntax error, so that can never happen.
test/csx.coffee
Outdated
|
|
||
| # Unlike in coffee-react-transform | ||
| test 'bare numbers not allowed', -> | ||
| throws -> CoffeeScript.compile '<div x=3 />' |
There was a problem hiding this comment.
I would like to see what the error messages look like for these, and move the tests to error_messages.coffee
There was a problem hiding this comment.
There's no reason to have these test both here and in error_messages.coffee, is it?
|
Random thoughts:
|
Correct on all points.
No, I think the main troubles came from a custom parser that was independent of CoffeeScript. Outputting JSX mainly simplifies the job of escaping stuff (since the escaping/unescaping will be done by the JSX transform).
Yes, I think so. To this change: at least it's quite unlikely that you will have code that previously compiled compile to something else. So it is a breaking change, but most people will be able to easily fix it.
There is nothing special in regards to comprehensions. You can use attribute strings without wrapping (currently broken, will fix and add a test).
You can see from the first TODO that indentation matter. After all, the tags are treated as object literal arguments. There might be a way to filter the indentation out, so it doesn't cause problems, but I don't think it's important to allow any formatting. CoffeeScript is an indentation based syntax, you should not expect CSX to suddenly ignore whitespace.
Spread is part of the JSX syntax and requires special support. Note that
There's a linter for ES with JSX too, right? Should be doable.
I will add this to the tests, but in short, they are pretty: I modelled the new errors I added after JSX errors, although the attribute one is prettier than JSX:
Literate CS uses indentation no? Shouldn't be a problem.
There are some tests for this ported over from coffee-react-transform. I am essentially passing everything through and allowing JSX to deal with this. |
|
This is indeed, the sort of "full-assed" support I was hoping for. Congrats! For what it's worth, I still think that CoffeeScript, as a concept, is made weaker by including JSX than leaving it out. But that said, if you fine folks want to put it in 2.0, this sort of patch is the way to do it. One note: Wouldn't it be better (and more harmonious) to have CSX leave out closing tags, and simply use CoffeeScript's significant whitespace to close all CSX tags? |
I like this idea. If we’re going to support JSX output, we might as well try to improve it (safely) however we can, just as we try to improve some of JavaScript’s rough edges. And I really want CSX to fit in with CoffeeScript’s overall use of significant whitespace, and not be another feature like chaining that creates a whitespace-insignificant zone in the middle of otherwise sensible code. I wonder if we could also find an alternate syntax that doesn’t use |
xixixao
force-pushed
the
csx
branch
3 times, most recently
from
b00697a to
ef1b8ba
Compare
|
Updates to code: Fixed comment. Used |
test/error_messages.coffee
Outdated
| ^^ | ||
| ''' | ||
|
|
||
| test "csx error: ambigious tag-like expression", -> |
There was a problem hiding this comment.
You mean ambiguous? 😆
There was a problem hiding this comment.
Yes! (Looks like I edited by comment too late 😀)
test/csx.coffee
Outdated
| test 'ambigious tag-like expression', -> | ||
| throws -> CoffeeScript.compile 'x = a <b > c' | ||
|
|
||
| test 'ambigious tag', -> |
|
@jashkenas I considered using whitespace. But there are theoretical and practical reasons not too. Theoretical:
Practical:
<div>
<div>Title</div>
<div className="bla">
Hello
</div>
world
</div>Now based on indent: <div>
<div>Title</div>
<div className="bla">
Hello
worldNow you could play with the rules and come up with some variations, but they will all be confusing/ambiguous/inconsistent. Think of JSX as heredocs (which is also, coincidentally, how I implemented it). You wouldn't want to use indentation there either, you also use delimiters """Hello #{
f(x)
} something""" |
|
@xixixao the next time you commit, can you please not force-push or amend? It’s easier to review this if each revision is a separate commit. All the commits will get squashed together when we merge the PR into |
|
@xixixao — While I'm not persuaded in the least by your two theoretical reasons, I am moved by your practical one:
I would love to see a sampler of 3-5 real-world examples of reasonable JSX templates, done in closed-tag and significant-whitespace styles. You may very well be right, but it would be worth evaluating. |
|
@GeoffreyBooth Github n00b, will do |
…r regex, fixed interpolation inside attributes value and added test
|
@GeoffreyBooth fixed, sorry about that. |
|
@xixixao @jashkenas @GeoffreyBooth @lydell I didn't see this until now, I've also been working on JSX-in-Coffeescript this week. I'll take a look at @xixixao's implementation but seems like I took a different approach: in addition to normal JSX syntax, there are whitespace-indented syntaxes (basically HAML) and it lexes differently inside of JSX elements but creates tokens and has grammar rules for them. To me that's more Coffeescript-y (it's certainly what I want for the same reasons I love Coffeescript) and I'm not surprised by @jashkenas' comment to that effect. I'll open a pull request as well (though I wasn't sure if merging it into Coffeescript proper (1.x or 2) would be the way to go, whether b/c of breaking changes, philosophical objections, ...) |
|
@helixbass thanks! It’s a shame that people are duplicating effort, but this makes clear just how in-demand this feature is. I’m more convinced than ever that we need to support JSX in a non-hacky way. Please target the @helixbass and @xixixao it might be worth looking at the Coffee Tags PR. It was an attempt to support a JSX-like syntax that used significant whitespace, and compiled out to Vue or React. |
|
Time to write the documentation for this. Anyone want to volunteer what it should be? Or at least what the example in the docs should be? Ideally it would be something that shows moderately complicated JSX, but takes advantage of one or more of CoffeeScript’s unique features such as array comprehensions. |
|
I have this component in my experimental project, if it's useful to you. It renders a rating like this:
|
|
@DanielRamosAcosta I’m trying to reduce your component to a short example. Can you tell me if this is correct? renderStarRating = ({ rating, maxStars }) ->
<aside title={"Rating: #{rating} of #{maxStars} stars"}>{
for wholeStars in [0...Math.floor(rating)]
<Star className="wholeStar" />
if rating % 1 isnt 0
<Star className="halfStar" />
for emptyStars in [Math.ceil(rating)...maxStars]
<Star className="emptyStar" />
}</aside> |
|
I'm only getting empty stars with that code... I'm looking into it |
|
(A lesser known feature of CoffeeScript is that you can leave out the for [1..3]
do knock) |
|
@GeoffreyBooth I've written some unit tests for my component and refactor with some of your improvements. The component you proposed I think it should be something like this: renderStarRating = ({ rating, maxStars }) ->
<aside title={"Rating: #{rating} of #{maxStars} stars"}>
{for wholeStar in [0...Math.floor(rating)]
<Star className="wholeStar" key={wholeStar} />}
{if rating % 1 isnt 0
<Star className="halfStar" />}
{for emptyStar in [Math.ceil(rating)...maxStars]
<Star className="emptyStar" key={emptyStar} />}
</aside> |
|
I see you use many more braces, which I was hoping to avoid. Are they necessary to get it to work? |
|
Yes 😢 , React only renders the return value of each statement inside the braces... |
|
Okay, I’ve updated my example: https://rawgit.com/GeoffreyBooth/coffeescript/2-docs/docs/v2/#jsx renderStarRating = ({ rating, maxStars }) ->
<aside title={"Rating: #{rating} of #{maxStars} stars"}>
{for [0...Math.floor(rating)]
<Star className="wholeStar" />}
{if rating % 1 isnt 0
<Star className="halfStar" />}
{for [Math.ceil(rating)...maxStars]
<Star className="emptyStar" />}
</aside>Does this work? |
|
That works, but you'll get React warnings becouse when you render arrays it is required to asign the "key" prop to the component. This is the warning:
|
|
Man, this is becoming a messy example then. I was hoping to use a loop comprehension with JSX, because loop comprehensions are something that ES lack (though |
|
@GeoffreyBooth |
|
@xixixao @GeoffreyBooth // props = {id: "a", title: "some title"}
<div {...props}></div>compiles to <div id="a" title="some title"></div>I don't have much experience with JSX, but it seems that |
|
Could we just compile <div a="x" {props...}></div>to <div a="x" {...props}></div>? Is that really an object spread in there, as opposed to generic spread syntax like we’d find in function parameters or an array? |
|
I was thinking the same. |
|
@GeoffreyBooth |
|
Here is a summary to the spread syntax in
// Spread the whole props object
<div {...props} />// Spread two props objects
<div {...props1} {...props2} />// Spread an updated props object
newProps = {...props, a: "x"}
<div {...newProps} />// Spread an updated props object within <>, not reliable, not recommended
<div { ...{...props, a: "x"} } />// Not supported. Currently wrong syntax
<div {...props, a: "x"} />// Not supported. Currently wrong syntax
<div {...props1, ...props2} />Accordingly, # Spread the whole props object
<div {props...} /># Spread two props objects
<div {props1...} {props2...} /># Spread an updated props object
newProps = {props..., a: "x"}
<div {newProps...} /># Spread an updated props object within <>, not reliable, not recommended
<div { {props..., a: "x"}... } /># Not supported. Currently wrong syntax
<div {props..., a: "x"} /># Not supported. Currently wrong syntax
<div {props1..., props2...} /> |
|
Implemented via #4607. |
|
Did you try https://github.com/askucher/lsxc ? Pug/Jade like syntax |
|
Looks like this really works: https://iainhouston.com/blog/exploring-react-with-coffeescript-2.html |
So here's a fully functional implementation of what I call CSX - the JSX equivalent for CoffeeScript. I tried to adhere as much as possible to the syntactical rules of JSX (as opposed to doing possible fancy stuff).
First, let me address why I think it theoretically makes sense to include this in CoffeeScript:
Second, I implemented this on top of
2, but the implementation would make it trivial to support it in original CoffeeScript too, provided we would have a way (option, pragma) to determine what target the CSX tags should compile to.So how is this implemented and why: JSX is a super simple transform, provided you know you are looking at an expression in the surrounding language. Conceptually it works like this:
and so on. Of course
Reactfor example actually usesSomething('div')instead ofdiv(), but that doesn't really matter for the parsing phase. So I actually turn CSX into valid CoffeeScript tokens in the lexer. Note that some of the work could be done in the rewriter, the split between the lexer and the rewriter is very arbitrary, and I just found it easier to do this in the lexer, instead of having to pass bunch of information to the rewriter. But I think it would be doable to move most stuff outside ofcsxTokenmethod there.Once I do this conversion, it's actually tricky to tell which nodes were originally CSX. The Jison parser does not make it easy at all to pass adhoc info from the lexer to
nodes(which compile the tree to JS), so I created a new token for the tag, and seed the info from there when compiling. The tagging of children nodes is not super clean, but I haven't done anything that wasn't at least a bit used before.Now, for differences between JSX, coffee-react-transform and CSX:
<xwith leading space is no longer supported. Supporting this leads to highly ambiguous syntax:both JSX and the transform handle this "correctly", allowing this code to be treated as comparison. I'm not sure that there is a way to allow this in CoffeeScript without making the resulting compilation ambiguous. JSX can do this easily, because
a <b/>is not a valid JS epxression, but in CS it is simplya(<b/>). I think the transform looks ahead to determine whether there is a closing tag, and we could do something similar, but I don't know how to do that without introducing incorrect edge cases.Ideas welcome.(Update: I think restricting this to only unbalanced whitespace should alleviate most concerns)You can see these in the tests I added. Note that I removed some original coffee-react-transform tests, which were mainly testing the CoffeeScript parsing, not the CSX part.
What's left?
Spreads, blocked by #3894
One case which is broken, which boils down to this being invalid CoffeeScript:
(there is a corresponding CSX test with a TODO).
Fixes #4529.