Skip to content
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

JSX: Whitespace in properties is lost #896

Closed
zpao opened this issue Jan 15, 2014 · 10 comments
Closed

JSX: Whitespace in properties is lost #896

zpao opened this issue Jan 15, 2014 · 10 comments

Comments

@zpao
Copy link
Member

zpao commented Jan 15, 2014

It looks like we're doing some current position moving incorrectly. @jeffmo, @syranide - do you know if this is something that's getting fixed with the whitespace PR or is this a separate problem?

<Component something="       a bug        " />

becomes

Component( {something:       " a bug "        } )
@zpao
Copy link
Member Author

zpao commented Jan 15, 2014

Play around with different amount of whitespace padding, as well as a whitespace only string. Looks like there are a few edge cases to handle here.

@syranide
Copy link
Contributor

@zpao I have intentionally fixed a few in my whitespace PR (including your example I believe) and I have intentionally not addressed some of them and there are probably a bunch of them that I haven't even come across (didn't look for them).

I think we need to decide on what we actually want in the output, should we try to be as faithful to the source as possible or should we always generate the "correct minimal syntax" loosely speaking?

@sophiebits
Copy link
Collaborator

I feel that for attributes we should maintain the original spacing exactly. Line breaks are a small pain but otherwise there's no issue.

@syranide
Copy link
Contributor

@spicyj But what does that actually mean, exactly? :)

<Child.value="5"/>
Child({value:."5"})
<Child..value=."5"./>
Child({.value:.."5".})

?

Intuitively that actually seems kind of ok, if we want to reproduce the source, except I'd do a special-case for the ./>:

<Child..value=."5" />
Child({.value:.."5"})

Also, we want to reproduce line-breaks exactly so that line-numbers match up, but I think my whitespace PR takes care of that (mostly) at least.

@sophiebits
Copy link
Collaborator

Sorry, I just meant that the text within the quotes should stay the same.

But I agree that it would be nice if this happened:

<Child.value="5"./>
Child({value:."5"})

Because that's surely the most common way of writing the JSX and corresponding JS by hand.

@syranide
Copy link
Contributor

@spicyj Ah right right, yes my whitespace PR fixes that I'm pretty sure, so what you put inside the quotes is exactly what ends up in the output code.

@plievone
Copy link
Contributor

This seems to be fixed in current master (by #480).

@sophiebits
Copy link
Collaborator

This is mostly fixed, though there seems to be some extraneous whitespace in the surrounding JS:

<Component something="       a bug        " />
Component( {something:       "       a bug        "        } )

@syranide do you know if #970 fixes this?

@syranide
Copy link
Contributor

@spicyj That PR should fix 99% of the output weirdness, so yes. :)

@sophiebits
Copy link
Collaborator

This is now fixed.

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

No branches or pull requests

4 participants