Put comma after any non-whitespace non-comment characters in XJSExpression #3129

jasom · 2015-02-12T04:28:03Z

For some reason the comma was emitted in renderXJSExpressionContainer if
it was an XJSExpressionContainer, and in the caller of the function
otherwise. This made getting things like <Foo bar=(baz) quux={biff} />
hard.

This should fix issue #1673

facebook-github-bot · 2015-02-12T04:29:42Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at [email protected]. Thanks!

jasom · 2015-02-12T04:30:57Z

Re: the TravisCI failure:

Hmm; that test passed locally; I'll check if I'm missing anything from the commit, and update.

jasom · 2015-02-12T05:42:45Z

Added in the missing change, and signed cla.

zpao · 2015-02-12T05:42:57Z

Hmm, can you run npm test locally - it looks like that might not be working on Travis (the grunt jest task doesn't seem to be outputting anything and is passing but running locally it fails). This will run our transform tests (we should also add one for whatever changes we have here, looks like there was a test case in the linked issue).

jasom · 2015-02-12T05:47:16Z

Ah, npm test is much better; I was doing a grunt test previously to test it, per the readme. It looks like it wants the comma before the following whitespace. Is this a correctness issue with ASI in the event of a newline, or just an aesthetic thing?

facebook-github-bot · 2015-02-12T05:49:34Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

jasom · 2015-02-12T06:59:27Z

The tests seem to think that for:

<foo> {
 bar
//comment
}
{ baz}
</foo>

The comma ought go immediately after bar. That surprises me; does anyone happen to know why this is the case? I can sort-of see putting the comma after the last non-whitespace item in the braces, but putting it before comments seems rather odd to me.

syranide · 2015-02-12T08:18:50Z

The reason for them being inlined into the sub visitors is to have control over where they end up. Moving it out like that works, but you then end up with commas after newlines/whitespace rather than before as you would write yourself.

jasom · 2015-02-12T08:52:38Z

@syranide Starting to get a bit off topic, but as far as "as you would write yourself." you probably wouldn't put a space right before a newline, so that becomes a question of how far you want to take it. I'm testing a patch that fixes #1673 without changing any existing output; it's a lot more complicated than this one though.

syranide · 2015-02-12T09:05:18Z

@jasom ~~I don't see how this is off-topic~~, the current output by JSX (except for whitespace before newline) was agreed on in #970 and follows common style-guides/lints and the way you would write it yourself, this PR seems to undo just that?

, \n is a deficiency in the current implementation due to the complexity of solving it (it seems).

jasom · 2015-02-12T09:08:32Z

@syranide I meant the , \n nitpick was off topic, not your comment.

syranide · 2015-02-12T09:34:42Z

@jasom Ah :)

jasom · 2015-02-12T17:00:54Z

Okay, I fixed the same issue in a completely different way; it should change not at all any whitespace placement. I also added a unit test for the issue I fixed.

syranide · 2015-02-12T17:10:59Z

@jasom Your commit is broken (it includes git comments).

syranide · 2015-02-12T17:17:53Z

vendor/fbtransform/transforms/xjs.js

+    var commaPos = 0;
+    //console.log('"'+value+'"')
+    for (var i=0;i<value.length;++i) {
+        switch (state) {


My vote goes to if instead of this nested switch nightmare.

jasom · 2015-02-12T17:24:19Z

@syranide Sorry, I pushed the wrong branch

jasom · 2015-02-12T17:32:26Z

@syranide Fixed up git garbage, converted switch to if/else. I have to go to work now so any other changes will have to wait until tonight.

zpao · 2015-02-12T19:28:22Z

cc @jeffmo

syranide · 2015-02-13T09:47:09Z

vendor/fbtransform/transforms/xjs.js

+    var state = "normal";
+    var commaPos = 0;
+    for (var i=0;i<value.length;++i) {
+        if (state === "normal") {


You have 3 else-branches with the same commaPos = i+1;, it should be possible to reduce and flatten this unless there's a very good reason for this verbosity.

Yeah, the state machine started out a bit differently, I'll clean that up. Also, if you know a better way of checking for whitespace than (foo.trim() === "") let me know.

Couple small style nits (can fixup later with anything else that might come):

2 space indent (match the rest of the files)

spaces around operators (for (var i = 0; i < value.length; ++i))

use === and semicolons consistently. The line below this is missing the former. Line 28 the latter.

@zpao I fixed those in all the places I found; is there a style linter I can use to check for this in the future?

You can run eslint with grunt eslint or grunt lint

@hzoo Yeah, I did that, but it didn't flag any of the style issues zpao pointed out.

Oh then it's probably because it's not being processed - yeah src/vendor is in the eslintignore.

# We can probably lint these later but not important at this point src/vendor

So instead you could try installing it globally and run it on a single file or remove the ignore from the file temporarily.

jasom · 2015-02-19T05:03:50Z

Is there anything else I need do to get this PR ready for merge?

zpao · 2015-02-19T23:33:51Z

There are still a few stylistic things. I'm in the process of enabling eslint here so that these files get linted and can tell you what's going on (namely use single quotes, match other styles with indentation, else bracing). If you rebase on top of #3206 (or it will be in master shortly I think) then you can see this yourself with grunt lint (that grunt task doesn't take params and is actually a bit noisy about long lines in other code so you could also run ./node_modules/.bin/eslint vendor/fbtransform directly)

jasom · 2015-02-20T00:53:33Z

The changes I made should be clean to eslint now (I ran it directly on
the file last night). Do you want me to clean up the entire jsx.js?

-Jason
On 15:34 Thu 19 Feb , Paul O’Shannessy wrote:

There are still a few stylistic things. I'm in the process of enabling
eslint here so that these files get linted and can tell you what's
going on (namely use single quotes, match other styles with
indentation, else bracing). If you rebase on top of [1]#3206 (or it
will be in master shortly I think) then you can see this yourself with
grunt lint (that grunt task doesn't take params and is actually a bit
noisy about long lines in other code so you could also run
./node_modules/.bin/eslint vendor/fbtransform directly)

—
Reply to this email directly or [2]view it on GitHub.

References

Lint vendor/fbtransform as well #3206

Put comma after any non-whitespace non-comment characters in XJSExpression #3129 (comment)

zpao · 2015-02-20T02:46:50Z

Please rebase and re-run. If you were running eslint from your branch point, it wasn't configured correctly. It has since been configured appropriately and I cleaned up everything else in the above PR. The remaining errors and warnings are a result of your changes (image below is with your branch rebased on master).

zpao · 2015-02-20T02:52:02Z

vendor/fbtransform/transforms/xjs.js

@@ -84,11 +121,13 @@ function renderXJSExpressionContainer(traverse, object, isLast, path, state) {
  if (!isLast && object.expression.type !== Syntax.XJSEmptyExpression) {
    // If we need to append a comma, make sure to do so after the expression.
    utils.catchup(object.expression.range[1], state, trimLeft);
-    utils.append(', ', state);
+    utils.catchup(object.range[1] -1, state, commaAfterLastParen)
+    //utils.append(', ', state);


Lint doesn't catch it but please remove this.

How embarrassing! I removed it now.

jasom · 2015-02-20T03:04:10Z

Thanks @zpao after rebase fixing the lint issues was easy.

jasom · 2015-02-20T06:36:54Z

Let me know if you want me to squash some of these cleanup commits together

syranide · 2015-02-20T08:10:41Z

vendor/fbtransform/transforms/xjs.js

+  var commaPos = 0;
+  for (var i = 0; i < value.length; ++i) {
+    if (state === 'normal') {
+      if (value.charAt(i) === '/') {


State-machine type of deal and all that... but this could be reduced to value.substr(i, 2) === '//', etc. @zpao?

I suppose it wouldn't surprise you that my day job uses mostly C...

jasom · 2015-02-25T06:07:45Z

Any other changes needed?

jasom · 2015-03-03T07:33:59Z

Just checking in on the status of this.

jasom · 2015-03-10T17:35:48Z

Just checking in to see if any action is needed by me on this.

zpao · 2015-03-10T18:10:04Z

Sorry, been focused on shipping 0.13. This won't make that cut but I'll come back to it after. In the mean time, this doesn't merge cleanly (probably mostly just because xjs.js is now jsx.js) so getting it back into a mergable state will help.

Should fix issue facebook#1673

@syranide

Requested by @syranide

This flattens the if/else structure quite a bit.

Also now ran eslint on this. The part that I added is now clean on eslint (the rest of the file not so much)

jasom · 2015-03-12T04:48:18Z

Now rebased on top of master

jasom · 2015-03-18T17:27:20Z

0.13 is out now. @zpao: Let me know if I'm being too annoying.

Put comma after any non-whitespace non-comment characters in JSXExpression

zpao · 2015-03-20T17:34:15Z

Squashed and landed as ef79679. Thanks and I'm sorry it took as long as it did!

Fixes facebook#1673 Closes facebook#3129

jasom force-pushed the master branch from ead3bc6 to e80e25c Compare February 12, 2015 05:35

jasom changed the title ~~Regularize emitting of comma on XJSExpressions~~ Put comma after any non-whitespace non-comment characters in XJSExpression Feb 12, 2015

syranide reviewed Feb 12, 2015
View reviewed changes

jasom force-pushed the master branch from 150ee66 to 033810e Compare February 12, 2015 17:25

syranide reviewed Feb 13, 2015
View reviewed changes

jasom force-pushed the master branch from 3183bcc to f025825 Compare February 19, 2015 04:43

zpao reviewed Feb 20, 2015
View reviewed changes

jasom force-pushed the master branch from f025825 to bb2671b Compare February 20, 2015 02:55

zpao added the JSX (React Flavored) label Feb 20, 2015

syranide reviewed Feb 20, 2015
View reviewed changes

jasom added 9 commits March 11, 2015 11:22

Put comma after any non-comments, non-whitespace in XJSEspression

0f7ca2b

Should fix issue facebook#1673

Convert switch statements to if/else

5d046e1

Requested by @syranide

Clean up comma-insertion transform

df2fdfb

This flattens the if/else structure quite a bit.

Style fixups from zpao

11b5da3

Also now ran eslint on this. The part that I added is now clean on eslint (the rest of the file not so much)

Fixed eslint errors

bc3ef5b

Remove commented out code

f0a9903

Test single-line comments as well as multiline

26f51e2

Simplify comma transform state machine

41685ff

Add in a missing space

8e1da6a

jasom force-pushed the master branch from f319613 to 8e1da6a Compare March 12, 2015 04:47

zpao added the GH Review: accepted label Mar 20, 2015

zpao closed this in ef79679 Mar 20, 2015

zpao added a commit that referenced this pull request Mar 20, 2015

Merge pull request #3129

7860554

Put comma after any non-whitespace non-comment characters in JSXExpression

Pouja pushed a commit to delftswa2014/react that referenced this pull request Mar 23, 2015

Put comma after any non-comments, non-whitespace in JSXEspression

eaa79fa

Fixes facebook#1673 Closes facebook#3129

Put comma after any non-whitespace non-comment characters in XJSExpression #3129

Put comma after any non-whitespace non-comment characters in XJSExpression #3129

Conversation

jasom commented Feb 12, 2015

facebook-github-bot commented Feb 12, 2015

jasom commented Feb 12, 2015

jasom commented Feb 12, 2015

zpao commented Feb 12, 2015

jasom commented Feb 12, 2015

facebook-github-bot commented Feb 12, 2015

jasom commented Feb 12, 2015

syranide commented Feb 12, 2015

jasom commented Feb 12, 2015

syranide commented Feb 12, 2015

jasom commented Feb 12, 2015

syranide commented Feb 12, 2015

jasom commented Feb 12, 2015

syranide commented Feb 12, 2015

Choose a reason for hiding this comment

jasom commented Feb 12, 2015

jasom commented Feb 12, 2015

zpao commented Feb 12, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasom commented Feb 19, 2015

zpao commented Feb 19, 2015

jasom commented Feb 20, 2015

zpao commented Feb 20, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasom commented Feb 20, 2015

jasom commented Feb 20, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasom commented Feb 25, 2015

jasom commented Mar 3, 2015

jasom commented Mar 10, 2015

zpao commented Mar 10, 2015

jasom commented Mar 12, 2015

jasom commented Mar 18, 2015

zpao commented Mar 20, 2015