Skip to content

Add single-line comment syntax to CJSX [Fixes #40] #41

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

Merged
merged 1 commit into from
Mar 12, 2015

Conversation

ConradIrwin
Copy link
Contributor

Before this change it was not possible to comment inside CJSX templates.

After this change, you can use the syntax:

 {#this is a cjsx comment}

The syntax was designed to look like a CJSX_ESCAPE that contains a
comment, but as the resulting coffee-script would be illegal this is
special-cased by the parser to return nothing.

Please let me know if you have any feedback on coding style, or feature
implementation.

FWIW: I also considered trying to special case CJSX escapes that contain
nothing but comments, but that would have required at least one extra new-line
for each comment, and would have caused things like {} to become valid syntax,
which I'm not sure is a good thing to do.

{# this is an empty CJSX ESCAPE
}

Before this change it was not possible to comment inside CJSX templates.

* jsdf#40
* jsdf/coffee-react#13

After this change, you can use the syntax:

     {#this is a cjsx comment}

The syntax was designed to look like a CJSX_ESCAPE that contains a
comment, but as the resulting coffee-script would be illegal this is
special-cased by the parser to return nothing.

Future work would be needed make this syntax work inside a tag:

    <Person {#comment} />

or to make multiline comments work:

    <Person>{###

    ###}</Person>
@ConradIrwin
Copy link
Contributor Author

@jsdf any feedback about this? re your comment at jsdf/coffee-react#13

@jsdf
Copy link
Owner

jsdf commented Mar 11, 2015

Yep sorry to keep you waiting, I started investigating supporting comments like those officially supported in JSX but got stuck on some other more general refactoring needed in the codebase, which has gotten to be a bit of a nightmare. To be honest though after looking into the way JSX now does it I'd rather just implement comments like this:

<div>
  {### this is a comment ###}
  {
    # this is also a comment, but you probably wouldn't use this style
  }
  <MyComponent
    someProp="a" # comments in props are fine
    ###
    also comments like this
    are another option
    ###
    someOtherProp="b"
  />
</div>

This reflects the behaviour supported in JSX, and is logically consistent with the way comments usually work in the language.

Thus the # single hash comments inside a {code block} would not be supported. However you could still use a single line {### herecomment ###}, which doesn't work currently due to a bug but could be fixed.

@ConradIrwin
Copy link
Contributor Author

Cool. I'll have a go at that syntax.

Unfortunately ### comments in coffee-script are much more limited than /* */ in javascript because they have to end at the end of the line, so we'll need to special case them in the same way to support this.

<div>{### comment ###}</div>

I'm not sure whether we need special cases for

{
# foo
} 

style; but I think we will (because CJSX expects {} to return a value, but comments aren't values).

@jsdf
Copy link
Owner

jsdf commented Mar 11, 2015

Oh you're totally right about the herecomment thing. I wasn't fully aware of that (though I vaguely remember running into it in the past).

In CJSX though, they get wrapped in parens which is compiled by coffeescript as the comment wrapped an anonymous function(?!) so you get this weird output:

React.createElement('div', (### comment ###))

becomes

React.createElement('div', ((function() {

  /* comment */
})()));

which looks pretty dumb, though if you're using UglifyJS with dead code elimination then it will become

React.createElement("div",void 0)

Still, we can probably do better by special casing it.

@jsdf
Copy link
Owner

jsdf commented Mar 11, 2015

We could introduce a different type of parse tree node CJSX_HERECOMMENT for comments appearing as children of CJSX_ATTRIBUTES or CJSX_ESC which could be output as escaped JS like

`/* comment */`

@jsdf
Copy link
Owner

jsdf commented Mar 11, 2015

Nevermind, coffeescript treats escaped JS as an expression so this won't work :/

@ConradIrwin
Copy link
Contributor Author

For the attributes ones we can probably just output coffeescript comments, because the newlines will happily line up.

The hard one is the comment-only CJSX_ESCAPE.

@jsdf
Copy link
Owner

jsdf commented Mar 11, 2015

Ugh coffeescript block comments are terrible. I didn't actually realise how it treats block comments as a statement rather than being equivalent to JS block comments (which aren't statements, just ignored text). Maybe your original approach is best.

@ConradIrwin
Copy link
Contributor Author

The thing I like most about the original request is that the comments look the nicest.

That said, I'd be keen to fix the comment-only CJSX_ESCAPE too (again probably by just not passing the comment through), though that doesn't have to be done at the same time.

@jsdf
Copy link
Owner

jsdf commented Mar 11, 2015

Okay I'm happy to merge this then, and I'll deal with the other cases seperately

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.

2 participants