ES6/7 Refactor

[amadeus]

Ok, this one has been a long time coming, and I finally found some time to sit down and work on it.

This PR refactors quite a bit of the syntax file to allow us to better support both current ES6 features, and make it far easier to support upcoming ES7 features.

The big thing it does is actually create many more distinctions around block types. By block types I mean anytime you use { and }. This includes being able to detect an if statement, vs a switch statement, vs a for/do/while loop, vs an actual object definition, vs function blocks, etc.

There are probably going to be some bugs.

One thing this PR does do, is provide a fix/support for:

var obj = {
    func(){},
    func2(){},
    [CONST.SOMETHING],
    [CONST.SOMETINGELSE]: function(){}
};

It also lays the framework for me to fix/add support for #433, #425, #393

I've made this branch on this repo, so please check it out and try using it so we can get it a bit more battle tested before merging it in.

[amadeus]

I've also reworked the file a bit to make it more organized, and tried to add more consistency with how we organize arguments and stuff. Please do not merge anything related to syntax files in until this gets merged in, or else it might be a nightmare for me to fix the merge conflicts.

[bounceme]

hey, just tried it out. by a quick test with indenting,since it relies on some syntax items, there seems to be a difference with line-comments. I couldn't pretend to be a great judge on the syntax side of things but i think you have done a great job! also seems faster, even with indenting

[amadeus]

I actually removed line comments as an item and just made them part of jsComment. Is this going to mess things up for you? I could put that back in.

[bounceme]

i'll try a few things out in terms of the line comment detection, i don't know for sure but maybe line comments are simple enough for a regex in the indent script

[amadeus]

I've gone ahead and made a first pass at supporting/fixing the following issues: #433, #425, #393

@iansinnott, @daGrevis, @paxunix if you guys can check out the es6-refactor branch and see if it fixes your issues, that would be super helpful. There will certainly bugs or things I have not yet forseen that still need to be fixed, but the more eyes on this the better.

[amadeus]

@bounceme @davidchambers I added a couple DISCUSS comments in the changes, would be curious to get your thoughts on it as well.

[davidchambers]

If we're to highlight these as errors, let's ensure they're not highlighted when used as property names. The throws in assert.throws, for example, should not be considered an error.

[bounceme]

these showing as warnings would help, but i think these should be detected by linters,unless we can make them errors only if used as identifiers

[amadeus]

I personally think we shouldn't even try to match or error correct these. I think leaving it up to a linter is a far better and more accurate solution. My vote would be to remove these completely since it's hard to figure what contexts they should and shouldn't be matched.

[davidchambers]

I think leaving it up to a linter is a far better and more accurate solution.

Seems like a nice separation of concerns to me. 👍

[daGrevis]

Bug in #393 seems to be fixed, good job!

Unfortunately I found a problem with HTML not being highlighted.

class MyComponent extends React.Component {

    // HTML syntax works if I remove this.
    state = {
        isActive: false,
    }

    render() {
        return (
            <div>
                HTML doesn't have syntax highlighting. Syntax breaks on ' character.
            </div>
        )
    }

}

[amadeus]

@daGrevis that's JSX syntax, which you can get proper syntax highlighting for using this plugin mxw/vim-jsx (it uses ours as a foundation, so it plays well).

Also, what you are seeing is a very tricky issue to fix. If you place a semicolon after that object definition, it should correct the issue you are seeing.

I am not sure there is valid/elegant way for me to fix that issue without requiring a semicolon

[kimfiedler]

I just tried the es6-refactor branch out on a few files in some of my projects and it seems to fix the issues with class instance fields and class static properties for me and I don't see any new problems.

It does - as you mentioned - require me to insert semicolons after object definitions. Would be great to find a solution for this but this is definitely a step up.

Thanks for working on this 👍

[daGrevis]

@daGrevis that's JSX syntax, which you can get proper syntax highlighting for using this plugin mxw/vim-jsx (it uses ours as a foundation, so it plays well).

Yes, I have that plugin already. Just saying that example above also breaks JSX highlighting.

Also, what you are seeing is a very tricky issue to fix. If you place a semicolon after that object definition, it should correct the issue you are seeing.

I am not sure there is valid/elegant way for me to fix that issue without requiring a semicolon

You are right, putting a semicolon at the end fixes the problem.

The thing is that me and a lot of other people are not using semicolons in JavaScript because they are optional. I'm not sure how I feel about cluttering the source code with them just to get correct syntax highlighting. Here's a commit where I added semicolons to a single file — quite a lot of noise. This also won't work for other people JavaScript code if they are not using semicolons.

What makes this issue so tricky to fix? Maybe we can put out heads together and try to fix it anyway? :)

Thanks for your work, I really appreciate it. 👍

[amadeus]

@daGrevis so it might be hard to give you a short answer as to why, but let me see what I can come up with here.

There's a high level discussion to be had for how accurate a syntax highlighter should be, but our philosophy has been that we want to catch obvious and simple syntax errors yet not get into more logical type problems that Flow, Tern or even ESLint would catch.

When creating a syntax regex in Vim, you have a few options - match, which takes a single regex and matches that to a groupname, keyword, which is basically a shorthand to match keywords without needing to write a regex, and finally the big one, region, which maps a specific region or scope based on a start and end regex.

Regions are special because they provide a new blank cope to work from that you can then opt other types of regions, matches or keywords into. There is one caveat with regions however, as long as the starting regex matches, the region begins. It does not care if there is an ending region or not.

Something we do with this syntax file is create a ton of regexes that we then group into a special cluster called jsExpression. As the name implies, these are basically any sort of expression in JS. this include a variable, string, function, object, equations, etc. We pass this cluster into various regions as needed.

Example regions include an object (defined by a { and closing }), function definition, (which is defined by a { and } as well, however it's scoped to ONLY come AFTER the ( and ) of a function definition.

Using these scopes allows us, for example, to differentiate between an object key someKey: and a more generic ternary expression test ? "true" : "false".

In the case of your example, we have a class definition block, also defined by { and }. Classes allow for special function type definitions like this (also, ES6 provides a similar shorthand syntax for objects):

class Example {
    // Inside here is a class definition region
    someFunc(){
        // Inside here is a function definition region
    }                   
}

This type of shorthand syntax is technically a syntax error if you put is in another context:

var func = someFunc(){};

In this case, someFunc() would be considered a function call of another function named someFunc, and the {} after it would technically evaluate to an object, this of course is a syntax error when it reaches the opening {.

Therefore, that specific regex to match a function definition, within a class block, is special, and isolated ONLY to the class blocks.

It gets complicated, because class definitions in ES7 syntax, support the ability to create expressions:

class Example {
    someVal = 20
}

This is cool and all, and you're probably thinking, why don't we just throw the entire jsExpression cluster inside there. Well this becomes a problem in a case like this:

var getValue = function(){
    return 2;
};
class Example {
    someVal = getValue()
    someFunc(){}
}

In this case, we need to match getValue() as a function call, but match someFunc as a function definition. If we just make jsExpression contained within the class block generically, then we will will run into a priority problem - whichever regex was defined later (function name or function call) will ALWAYS take precedence. So either you will ALWAYS get matched as a function call or a function definition.

The way to fix this is to create a new special region (called in this case jsClassValue) that then follows an = sign WITHIN a class block that then contains the jsExpression cluster. This works great up until we have to figure out how to END that region. If you remember from earlier, the region will ALWAYS start, regardless of whether it finds an end or not. The = provides our start, however now we have to end that region. A semi colon makes it super easy, since we don't have to worry if newlines are involved, AND other contained regions inside of this new region will then extend the jsClassValue region conveniently. By this I mean if you did something like this:

class Example {
    getValue = () => {
        return this._value;
    };
    someFunc(){}
}

The semicolon within the arrow method will not end the jsClassValue region. And it won't be until we end the function block region defined by the { and } that we match the closing ; for jsClassValue region.

Javascript is introducing so many new fancy shorthand features, that it's becoming a huge nightmare to match using regexes.

Maybe others have some ideas?

[amadeus]

Having said all that, I have just pushed a small change to add a newline \n as a region end to jsClassValue, and I think this may actually work in most cases.

There is still the possibility of a sort of multiline statement breaking things, but I think at least for now, it could help.

Please pull the latest changes and test them when you have a chance.

[daGrevis]

I can confirm that it works for every case in svg-scene repo. I also tested it on my other private ES6 + JSX repo and there were no problems.

I will test it at work on Monday.

Thank you! 💯

[amadeus]

Alright, I think this PR may be ready to go in. Obviously it's still for develop, so there is more time to fix things. Any objections to getting this in now?

[iansinnott]

Excellent work @amadeus! So far this is looking great