-
Notifications
You must be signed in to change notification settings - Fork 131
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
Extend JSXText with Comment? #7
Comments
If we do this then we should treat all text and attributes as JS encoded (i.e. allow backslash escapes) and not have special support HTML-entities. Another alternative is to support XML style comments instead I think we need to make up our mind. Are we JS or are we XML? @jeffmo |
@sebmarkbage Yep, I explored that idea a bit in #4. To me it makes perfect sense to drop HTML-entities and not pretend that JSX is HTML, JSX is great for any UI frontend, HTML just happens to be what the web is built on and what React currently targets. As I mention in that issue, it would also put an end to the weird My personal opinion on In my opinion JSX is currently 50% JS and 50% XML, being 100% JS would certainly be preferable. But it's your decision obviously :) var result = <div value="a">text{<div />}<div />text</div>;
/*var result = <div value="a">text{<div />}<div />text</div>;*/
var result = /*<div value="a">text{<div />}<div />text</div>;*/
var result = <div /*value="a"*/>text{<div />}<div />text</div>;
var result = <div value="a">text{/*<div />*/}<div />text</div>;
var result = <div value="a"><!-- text{<div />}<div />text --></div>;
var result = <div value="a">text{<div />}<!--<div />-->text</div>; ... is kind of weird. |
I agree about |
I am human. I am dancer. |
There's no technical reason we couldn't support . The only reason we haven't is because we support comments in {} expressions (which was technically simpler and covered more flexibility). My personal opinion: We should stick with the status quo. If we go with first class comments like this, we make it easy to erase text unintentionally (by accidentally having a '/*' or '//' in your text somewhere. Doesn't seem worth that to me...but I'll leave it up to the popular opinion to decide if everyone else agrees. |
I also don't think is really that much better than {/* */} (It's the same number of chars even) |
Oh...this issue is super old... |
@jeffmo I still think it's relevant though. But my point was that why even require PS. I also think that |
@camronjroe: Having a parser flag to turn this on/off would be a not so great idea. It would mean react component interop (meaning like sharing react components) wouldn't just work -- you'd have to worry about the parser settings each component relies on too. It would also mean weird little edge case bugs (like text not showing up, or showin up when you don't want) could pop up in application code that ultimately traces allll the way down out of your app code and into your build/compilation system. Turtles! |
@syranide: I won't die on a hill against this, and I agree it's /maybe/ not common to have // or /*, but it's the times where it does that would make this really frustrating (it's easy to miss, and because it doesn't happen often...really hard to track down). Anyway, |
@jeffmo But that's partially what syntax highlighting is there for right? :) |
@jeffmo In that case a flag may not be the way to go. Considering the use of The problems here are that js can't be fully embedded properly and it takes about 2+ seconds to do |
What I find mostly annoying about the current state of comments is that, this makes sense and works:
But this doesn't:
Instead you have to do:
While they're largely the same, there's a mental burden in having to check the context and Then there's also the problem that the current expression comments acts as a separator, so you cannot comment out in the middle of text without affecting the resulting children. It's not really all that common and a big deal, but perhaps yet another reason for why it's a bit weird.
|
Thank you for reporting this issue and appreciate your patience. We've notified the core team for an update on this issue. We're looking for a response within the next 30 days or the issue may be closed. |
I am for XML comments as they make it much easier to comment something out in the JSX flow and keep it friendly to the human eye at the same time. <Foo>
<!-- I will tell you why this component is here -->
<Bar>
</Bar>
</Foo> This would also allow us to quickly temporarily comment out components in the JSX flow like this: <Foo>
<!--Bar>
</Bar-->
</Foo> |
@jeffmo said "I also don't think is really that much better than {/* */} (It's the same number of chars even)". Technically it is one more character, but it doesn't have to be. For example, If I want to comment out this block of JSX:
I would do it by adding just five characters like this:
But the number of characters isn't the point. I think their are four points.
Please reconsider this. I'd be willing to work on a pull request to add support, but only if there's a good chance of it being accepted. |
@mvolkmann IMHO, JSX is not XML, it's JS. It's just new syntax to do something we've always been able to do. <!--div>
<div>some text</div>
</div--> Is also problematic, because that would mean that JS(X) now has two ways to comment stuff.
|
👍
@syranide Given that JSX is a "new syntax to do something we've always been able to do", wouldn't this argument (JSX has two ways...) be an argument against JSX entirely? There's already "two ways" (at least) to do almost everything in JS(X), the plain JS way or with the new syntax. Why should comments be different? This recommendation (as I understand it) is simply to make the second (X) way more consistent with XML, which is the expectation of almost any coder learning JS(X) in my opinion. It lowers the barrier to entry and makes for fewer awkward well, no, that actually doesn't work like you would expect moments when introducing folks to React. |
True, but the syntax we had for it is in many ways bad, it was hard to read especially for large hierarchies and cumbersome to write. It's also very alien to typical designers. There are also some technical differences. Yes, there's some overlap there with
JSX elements is already different enough from HTML that learning to use JS-comments for commenting code should seem trivial, especially since you need to have some level of experience in JS anyway. The benefit of having a single way to comment is also important IMHO, see #7 (comment). Also worth noting, I think it's one of those things that makes intuitive sense, but when you get down to it and actually start to evaluate the side-effects it no longer seems like good idea. That being said, if |
That its not where the XML comment would be allowed to work. Everybody coming from HTML will naturally try XML comments like they did in HTML. And they ask will fail when trying this in jsx. I did. You can explain them all day long that jsx is js and not html although it looks like html from an birds eye view. And while XML comments would be natural, they aren't JS. Would be interesting to run a questionnaire with beginners about their first try to comment some jsx markup. |
@pke But then you have to use different comment syntax depending on where are you in the code... having contextual comment syntax seems like over-complicating a fundamental language feature when the syntax we already have would work just as well. It also hurts refactoring.
|
Oftentimes you actually need comments emitted in HTML. Such as adding notices into the source, debugging crazy off-by-one errors, inserting markers in the output for post-processing. Forcing this kludge is against all that is human: <div style={{display:none}} dangerouslySetInnerHTML={{__html: `
<!-- here is my comment
it was a hard fight wasn't it?
---> `}}></div> |
Contextual comments are not just a mental burden, they completely break all pre-existing comment shortcuts in code editors. I normally never actually type AFAICT just about every code editor has a feature like this. And they all won't work in JSX, and it would be unreasonable to put the burden of "fixing" this issue on them when it's the – not part of the standard defining .js files – JSX that created this issue.
JSX is syntax used across multiple libraries (it's not only React) in several environments that are not the dom (most of my JSX work lately has actually be in React Native where there is no DOM; there are also other react renderers – a three.js canvas renderer, Blessed terminal interfaces, Titanium mobile apps, x11 windows, PDFs, Ionize/Electron apps, ...; I'm actually surprised no-one has made an "jsx2xml/ReactXML renderer" yet). I don't think we should add HTML comment nodes to the JSX syntax itself, especially when the actual use for explicit comment nodes is very small. However, for when you do explicitly want a comment inside your html I do think it would be reasonable for import {Comment} from 'react-dom';
<div>
<Comment text='here is my comment' />
</div> |
It may be worth extending
JSXText
with a case forComment
. In a way it's a bit weird as it is technically as if the comment in'abc/*def*/ghi'
would actually work. But practically, I think it makes sense, commenting out JSXElements is currently quite unintuitive/hacky as you have to write:Note that comments actually affect (split) the children too, rather than being transparent. I'm proposing extending the syntax to support the following:
As I mentioned, it is a bit weird in a way but still rather intuitive and useful, especially in the sense of having JSX feel like a true extension of JS and not just sugar. I think there is little to lose here;
/*
,*/
and//
are unlikely to occur in text, if they do then HTML-entities or JS-escaping (#4) to the rescue. Or if you don't care about children{'/*'}
(half-related #6, leading/tailing whitespace).The text was updated successfully, but these errors were encountered: