pangloss / vim-javascript

Vastly improved Javascript indentation and syntax support in Vim.
http://www.vim.org/scripts/script.php?script_id=4452
3.8k stars 358 forks source link

Wrong indentation #618

Closed sassanh closed 8 years ago

sassanh commented 8 years ago

I'm editing some jsx files and am using vim-javascript and vim-jsx. It was all fine till around 10 days ago something went wrong. Consider this:

    return <div
        style={{
            x: 1,
                y: 2,    //   Wrong indentation ######################
                z: 2,    //   Wrong indentation ######################
                w: 2,    //   Wrong indentation ######################
        }}>
            123
        </div>;

Look at the lines I marked. If I change double {{ with single { it works alright but obviously it doesn't compile.

bounceme commented 8 years ago

So why use two curlies? I don't know whether that is css or xml or js, but in js it's a syntax error

bounceme commented 8 years ago

oh nevermind, it's supposed to be a js object within the jsx. but that would likely be an issue for mxw/vim-jsx

amadeus commented 8 years ago

@bounceme so in JSX, the first { and } are used to define where you can write actual JS. The second ones are actually the object itself.

i.e.

() => {
    return <div
        defaultStringProp="some string"
        anotherStringProp={"another string"}
        aNumberProp={12}
        anArrayProp={[1, 2, 3]}
        anObject={{ key: 'val' }}>
  </div>;
}

This would be invalid JSX, and would throw an error:

() => {
    return <div aNumberProp={key: 'value'}></div>;
}
amadeus commented 8 years ago

Not sure exactly how it's supposed to work, but we might have to build special casing for being within a JSX region. JSX regions start whenever you have a < immediately followed by a letter.

So <a would be the start of a jsx region, and it won't end until it finds a matching </ that is followed by the starting letters from <a.

Within that syntax though, anytime you do { and } it is basically a way to say, forget the JSX syntax, I want to write JS now. So all JS indentation rules should apply at that point. Perhaps you can do some sort of special case lookup in the indent script that detects a jsBlock within a jsxRegion?

bounceme commented 8 years ago

The real issue here in javascript(possibly not in es6) ,there isn't any valid place to have two opening curlies which delimit an object, so parsers and whatnot get confused by the { token.

I could add the curly here https://github.com/pangloss/vim-javascript/blob/master/indent/javascript.vim#L86 but I need to make sure it doesn't conflict with regular js first

sassanh commented 8 years ago

Thanks for quick replies guys. As you mentioned it's jsx, the weird thing is first line has correct indent but from second line it goes wrong. vim-jsx is updated rarely (compared to develop branch of vim-javascript which I use.) So I'm pretty sure it happened after one of vim-javascript updates, as I said around 10 or 15 days ago. I just thought it'll get fixed in some new version but never happened.

Btw, generally it'd be super if vim-javascript and vim-jsx could merge together somehow as vim-jsx relies on vim-javascript a lot. But I don't know if that's possible at all.

bounceme commented 8 years ago

You can just add a { to this regex part [-=~!<*+,.?^%|&\[(] . this makes the indenter consider the curlies as object braces

sassanh commented 8 years ago

Thank you, it solved it. Is it the clean solution and is going to be in coming releases or should I just keep it for myself?

bounceme commented 8 years ago

should be clean, as long as things like this aren't considered blocks (or are just errors):

if (){
  { '1': 1 }
}
bounceme commented 8 years ago

actually not:

if (9){
    { 
        console.log(5)
    }
}

this is valid js (edit: formatting)

sassanh commented 8 years ago

Yeah, then I guess I should solve this in vim-jsx. Or is there anyway you apply vim-javascript indentation only for things that are in js-blocks? this way first brace doesn't affect the indentation.

bounceme commented 8 years ago

Not that I know of, i'll take a look at the jsx plugin later

sassanh commented 8 years ago

That'll be kind of you.

bounceme commented 8 years ago

hey, I think #619 may fix this. It is convenient that jsx doesn't support block statements!

sassanh commented 8 years ago

Yes, it fixed the issue.

It is convenient that jsx doesn't support block statements!

I think so too.

There's another minor issue with jsx files, I describe it here as it's relevant to this context. Let me know if I should create a separate issue for it. This is a sample of the incorrect indentation:

<div
    key={isItDay
        ? '123'
            : '123' // <--- WRONG INDENTATION ######################
    }>

I thought maybe this is related to the js-block issue and its relation to jsx blocks we talked about earlier on this thread.

bounceme commented 8 years ago

You'll have to change one of these: https://github.com/pangloss/vim-javascript/blob/develop/indent/javascript.vim#L63

what needs to be confirmed before i can merge that pr is whether it's possible to have object braces preceded immediately by block braces, e.g.

if () {
  {a:1}
}

or

if () {
  console.log()
  {a:1}
}
bounceme commented 8 years ago

and it seems like it:

// both errors, indenting after comma
if (9) {
        console.log(9)
        {
                a:1,
                        b:2
        }
}
if (9) {
        {
                a:1,
                        b:2
        }
}
sassanh commented 8 years ago

This is how it goes in jsx branch:

if (test) {
    {
        console.log(123);
        console.log(321);
    }
}
if (test2) {
    {
        a: 1,
            b: 2, // <--- WRONG INDENTATION ###################
    }
}

Second one is incorrect.

Though now I checked it without '[:{]\_s*\%#' (with ':\_s*\%#') and it still indents the same. And my compiler tell me the line I marked wrong above has syntax problems (b is unexpected token.) so I guess it's not valid js (due to ambiguity on if it's a js block or a js object). we should wrap it in parenthesis:

if (test2) {
    {
        a: 1,
        b: 2,
    }
}

Then the new commit indents it alright while old version doesn't indent it alright. So seems like the problem this commit fixes wasn't only related to jsx regions.

sassanh commented 8 years ago

and it seems like it:

Your second example is not valid js. We should only take care of first one.

bounceme commented 8 years ago

both aren't valid, that is reflected by the broken indenting

if (test) {
    {
        console.log(123);
        console.log(321);
    }
}

this is valid though ^^

sassanh commented 8 years ago

yeah exactly. first one wasn't valid too. So I guess the PR is alright.

sassanh commented 8 years ago

You'll have to change one of these: https://github.com/pangloss/vim-javascript/blob/develop/indent/javascript.vim#L63

The problem is it works alright without changing these values with regular js files. It only happens in jsx regions. So I doubt if a continuation or opfirst character is actually missing.

bounceme commented 8 years ago

this is the result without making the filetype jsx:

<div
key={isItDay
        ? '123'
                : '123' // <--- WRONG INDENTATION ######################
}>
bounceme commented 8 years ago

Double indentation (for the first line with the ?), or no additional indent for the next line is 'wrong' indenting

sassanh commented 8 years ago

By regular js file I meant regular js file content (no jsx syntax) like this:

var a = true
    ? 123
    : 321

The above is using same javascript_opfirst and same javascript_continuation.

I found that it isn't really related to ternary if. Look at this:

{true
    <div>
        123
    </div>
        123 // <--------------------
}
bounceme commented 8 years ago

yea, > is an operator

sassanh commented 8 years ago

I see. But removing it from g:javascript_continuation didn't solve it.

bounceme commented 8 years ago

remove =\@<!> from g:javascript_continuation

sassanh commented 8 years ago

Even this doesn't work:

{true
    1
        2 // <--------------
}
sassanh commented 8 years ago

remove =\@<!> from g:javascript_continuation

Didn't work

bounceme commented 8 years ago

what would be causing extra indent in front of 2?

bounceme commented 8 years ago

https://github.com/pangloss/vim-javascript/issues/618#issuecomment-243973816

what is the result? remove =>\@! from opfirst and all other [<>]

sassanh commented 8 years ago

what would be causing extra indent in front of 2?

Nvm, I don't know how it happened, I can't reproduce it.

bounceme commented 8 years ago

filetype=javascript or jsx

{true
        <div>
                123 // indented because of > above
                </div> // indented because of operator first '<'
                123 // indented because of > above
}

This exhibits a bug with the jsx plugin

sassanh commented 8 years ago

what is the result? remove =>\@! from opfirst and all other [<>]

this is my opfirst now:

  let g:javascript_opfirst = '\%([,:?^%|*&]\|\/[^/*]\|\([-.+]\)\1\@!\|in\%(stanceof\)\=\>\)'

It's still the same, the wrong indentation is exactly the same.

{true

123 // indented because of > above
// indented because of operator first '<' 123 // indented because of > above }

I guess it's because you don't have vim-jsx. The </div> has correct indention for me. Try this instead:

{true
    ? 1
        : 2
}
bounceme commented 8 years ago

sorry, I can't reproduce your issues

//mine (tab = 8 spaces)
{true

        123 // indented because of > above
        // indented because of operator first '<'
        123 // indented because of > above
}

intentional:

{true
    ? 1
        : 2
}
sassanh commented 8 years ago

Ok, after removing : from g:javascript_opfirst and \|=\@<!> from g:javascript_continuation it's working alright. Thank you so much.

I just wonder if it's the elegant way or not? 😄 (Ofc I'll put these changes in my vimrc and not in the plugin itself.)

bounceme commented 8 years ago

It is the only way to satisfy everyone,imagine having global options for every type of operator hah

bounceme commented 8 years ago

I'll merge after i better confirm https://github.com/pangloss/vim-javascript/issues/618#issuecomment-243971244

sassanh commented 8 years ago

@bounceme dude I just wanted to thank you, last month my experience with coding js in vim was fantastic. It's just perfect now. I really appreciate your job on it.

bounceme commented 8 years ago

Pleased to hear it, and I had a lot of help from your suggestions!