microsoft / vscode

Visual Studio Code
https://code.visualstudio.com
MIT License
162.92k stars 28.76k forks source link

0.10.10 erroring on object spread properties in .js files #3804

Closed garthk closed 7 years ago

garthk commented 8 years ago

… and I can't turn it off by setting javascript.validate.enable false.

Perhaps this is a soft documentation bug: the release notes proudly mention experimentalObjectRestSpread but then say:

Support for ObjectRestSpread is not yet provided by Salsa but it is on the roadmap (see Microsoft/TypeScript#2103).

… which is confusing and misleading.

It's confusing because the stand-out tone of core dev responses to that issue is "not until TC39 moves it to stage 3; bother them, not us", which is much less optimistic than the Feb 2016 release notes or the TypeScript Roadmap itself.

It's misleading because it fails to describe the impact of "support… not yet provided" as error messages you can't make go away. If there's some way to make them go away, please describe it in that part of the release notes. If not, I don't think it's too strong to outright advise users of object spread properties to delay upgrading.

Without a work-around, this kills code for me, as I now have to dig for the real errors amongst a few dozen erroneous:

I trust a workaround is possible, as code is not erroring in JSX.

kumarharsh commented 8 years ago

+1

dlong500 commented 8 years ago

I'm happy for the inclusion of the Typescript-based Salsa syntax support, but as @garthk mentions it is an odd situation that we are now in. ESLint is also very much encouraged in the vscode docs (and I've been using it for quite some time already).

What I don't understand is why we can't have Salsa-based syntax color support (and maybe some other things as well) without forcing the Salsa (Typescript) syntax validation. It seems to me that if ESLint is to become the standard (or at least recommended) way for validation then there should be a simple way to disable the Salsa/Typescript validation. Apparently at the moment there is not, and this is a very serious impediment to productivity.

The issues with incomplete syntax color coding in past versions is highly preferable to an inability to disable code validation that isn't up to par with current coding conventions. The musings about specs and javascript stages on the Typescript repo in regards to the object spread operator don't justify a continued lack of support for a feature that is well-established in practice and is supported in Babel. That's fine if the Typescript devs want to hold back, but now Microsoft is forcing the Typescript opinions on everyone using vscode without an ability to disable it. I guess I just don't understand why the original javascript.validate.enable option can't be used to disable Typescript validation as well. After all, if we're using ESLint why do we need another validator?

There really doesn't seem to be a whole lot of noise about it yet, but that's probably because the February release only just came out. Salsa support in the January release was opt-in, and given the hoops you had to jump through to enable it I'm thinking that not a whole lot of people tried it out.

If I'm missing something then please enlighten me, but I haven't found any workaround yet (not even in the vscode alpha release).

Sorry if it seems like I'm cranky. I also just spent several hours figuring out that ESLint 2.3.0 breaks all linting in vscode at the moment.

josebalius commented 8 years ago

I feel the same way :/ I was looking forward to the Feb release to maybe switch from Atom but this is a blocker for me.

egamma commented 8 years ago

@garthk I will change the doc to make this limitation more obvious.

egamma commented 8 years ago

We will revive the javascript.validate.enable setting Microsoft/vscode#3909.

This will remove the red squiggles, but this will not fix that Salsa doesn't understand these syntactic constructs. This means the code understanding experience is limited, e.g, when it comes to intellisense, go to definition.

@dlong500 @garthk from this feedback I conclude that these limitations are tolerable as long as the red squiggles do not get in your way. Is this correct?

kumarharsh commented 8 years ago

these limitations are tolerable as long as the red squiggles do not get in your way. Is this correct?

Yes, you can say that. That's one major irritant because it's expected that linting run smoothly when transitioning from other editors. Linting and intellisense are separate things (from the user's perspective), and I think it's a more logical tradeoff that we'd lose intellisense support until Salsa comes up to speed.

egamma commented 8 years ago

@kumarharsh this explanation makes sense, thanks.

josebalius commented 8 years ago

@egamma reviving that setting would be great. I think it allows me to fully switch to vscode :)

dlong500 commented 8 years ago

How is the intellisense and syntactic recognition of Salsa different from the pre-Salsa engine? If there isn't much difference then I'm fine with simply disabling Salsa's validation. However, if there is a big difference would it be possible to have an option to continue to use the pre-Salsa engine for intellisense at least until Salsa matures a bit?

garthk commented 8 years ago

@egamma removing the red squiggles and entries in showErrorsWarnings (⌘⇧M) will help a great deal, catching code back up to subl in workability.

FWIW, goToDeclaration and triggerSuggest are still limping along. Nice work, that.

egamma commented 8 years ago

@dlong500 both Salsa and the previous JS infrastructure build on the TypeScript parser and AST. The previous JS infrastructure has the same limitations when it comes to using ES7 language features, but it supported to turn off validation.

One issue is that the TypeScript version used for the previous JS infrastructure is now really out of date. This is why we need to deprecate it as soon as possible.

until Salsa matures a bit? Do you have particular concerns with regard to the Salsa maturity.

egamma commented 8 years ago

@garthk the plan is to bring back the option and it will be in next update in the insiders channel, which we plan to make next week.

dlong500 commented 8 years ago

both Salsa and the previous JS infrastructure build on the TypeScript parser and AST. The previous JS infrastructure has the same limitations when it comes to using ES7 language features, but it supported to turn off validation.

Ok, thanks for the clarification. I just wanted to know if the pre-Salsa intellisense was superior but it sounds like it was the same or worse, so there's no need to keep it around anymore.

Do you have particular concerns with regard to the Salsa maturity.

Maturity maybe wasn't the best word. I was referring to the Typescript foundation of Salsa in regards to support for newer ECMAScript features that are already supported by other tools like Babel and ESLint. I get the desire to be careful with language constructs that may still be in flux, but I think sometimes there is too much hesitation in supporting features that are already used in the wild and are clearly going to be in a future spec one way or another.

In any case, thanks for the quick responses and for the revival of the option to turn off built-in validation.

Quick question: what is the difference between the insider channel and the alpha channel? Is the alpha channel always more up-to-date than the insider channel?

egamma commented 8 years ago

Typescript foundation of Salsa in regards to support for newer ECMAScript features that are already supported by other tools like Babel and ESLint.

got it, thans. This topic is discussed here https://github.com/Microsoft/TypeScript/issues/2103#issuecomment-182195915

Quick question: what is the difference between the insider channel and the alpha channel? Is the alpha channel always more up-to-date than the insider channel?

The alpha channel is updated multiple times a day and it is the version the development team is using. So it might be broken from time to time. The insiders channel is updated one week before a stable update, but we are looking into updating stable more frequently.

robcaldecott commented 8 years ago

I'm now on 0.10.11-insiders but no sign of javascript.validate.enable. Did it not make the cut?

egamma commented 8 years ago

@robcaldecott

I'm now on 0.10.11-insiders but no sign of javascript.validate.enable. Did it not make the cut?

the 0.10.11 insiders only includes the same critical fixes that went into stable. We look into pushing another insider update today.

egamma commented 8 years ago

FYI the insiders build with the above change is out now https://code.visualstudio.com/blogs/2016/02/01/introducing_insiders_build

kumarharsh commented 8 years ago

The linter seems to be working, but all the errors are being shown as: Parsing error: unexpected token (null)

egamma commented 8 years ago

@kumarharsh do you have the eslint parser options configured for JSX/React? See below:

Also the error message should be prefixed with [ESLint], this has been changed recently. If this is not the case can you update the vscode-eslint extension.

See the 'jsx' and plugins below

{
    "env": {
        "browser": true,
        "commonjs": true,
        "es6": true
    },
    "extends": "eslint:recommended",
    "parserOptions": {
        "ecmaFeatures": {
            "experimentalObjectRestSpread": true,
            "jsx": true
        },
        "sourceType": "module"
    },
    "plugins": [
        "react"
    ],
kumarharsh commented 8 years ago

@egamma updating the plugin fixed it. Actually, not I understood what that error is saying:

The eslint plugin highlights the ---- part on this object:

borderTopRightRadius: geometry.input.radius
borderBottomRightRadius: geometry.input.radius,
-----------------------

with the error:

parsing error: unexpected token (null)

I thought this was a because maybe the eslint plugin was throwing an error due to some internal error, but it is trying to find an eslint rule matching the missing comma and finds nothing, so it outputs (null). Maybe that's something which can be fixed?

egamma commented 8 years ago

The vscode extension shows the message it gets from eslint and when there is a rule then it is shown

image

There is no rule for parsing errors and this is why eslint shows (null), which is not very friendly...

image

I suggest to file an issue against the eslint repo

garthk commented 8 years ago

I already gave a thumbs on on the change being available in Insiders, but came back especially to call out the March 2016 release note content on this issue. Well writ, @egamma and @gregvanl. Thanks.

I'm full time on Insiders. It's looking good. javascript.validate.enable was all I needed. Thanks for turning that around so fast.

frogcjn commented 8 years ago

VSCode version 1.2.0 still has this problem?

const Button = (props, { theme }) => {
  const styles = getStyles(props, { ...config, ...theme });
}

[js] Property assignment expected.

kumarharsh commented 8 years ago

@frogcjn I'm sorry but I'm not getting this issue at all. Haven't gotten this issue after fixing up these issues:

  1. Open the folder with the eslint module in the node_modules folder directly under the root folder.
  2. Put the proper jsconfig.json Here's mine:
{
  "compilerOptions": {
    "target": "ES6",
    "module": "es2015",
    "experimentalDecorators": true
  },
  "exclude": ["node_modules", "scripts"]
}
frogcjn commented 8 years ago

@kumarharsh Could you tell me what is your typescript version?

I've tested. If you do not turn "javascript.validate.enable" off, it will always warn [js] Property assignment expected. Just as the document writes: https://code.visualstudio.com/docs/languages/javascript#_jsx-and-react-native

React Native examples often use the experimental Object Rest/Spread operator. This is not yet supported by VS Code. If you want to use it, it is recommended that you disable the built-in syntax checking (see below).

kumarharsh commented 8 years ago

@frogcjn yes, you'll need to turn off javascript.validate.enable flag, only then will eslint be able to properly lint your files.

frogcjn commented 8 years ago

@kumarharsh, so the VSCode Salsa currently still not support Object Spread/Rest? Right?

kumarharsh commented 8 years ago

Yes, I think that has been clarified by the vscode dev team time and again. Salsa does not have es7 features, but will get it later (most probably in this year). Anyways, disabling salsa does not take away too much from the experience - the intellisense still works okay (without some features), and even gives great cross-file suggestions like say I imported constants from a 'utils/constants' file, then intellisense shows those constants in suggestions, etc.

mjbvz commented 7 years ago

We just picked up a new release of TypeScript (2.1.4-insiders) that I believes fixes this. You can try this out in the current VSCode insiders build (2016-12-05T07:03:07.371Z) and it will also be going in to VSCode 1.8.

screen shot 2016-12-05 at 4 15 31 pm

Please give the latest insiders builds a try and let us know if you run into any trouble or notice any problems.

Thanks

lizhiyao commented 7 years ago

I found that Object Spread/Rest not supported in .vue files.

.vue files

zfeher commented 7 years ago

Regarding the .vue files object/rest support the Vue Component extension may help.

As I see the .vue files are recognized as HTML by vscode so the HTML syntax need to be extended with object rest/spread support.

tforssander commented 7 years ago

@zoltanzf Thanks for the tip, this issue have been bugging me for a while!

lizhiyao commented 7 years ago

My solution is vetur.

ronjouch commented 7 years ago

My solution is vetur.

@lizhiyao vetur works for you? How? (Any particular setup / are you sure you didn't just disable JS validation?) I just tried vetur 2.0.1 with a fresh profile on 1.8.1 and today's 1.9.0-insider, and I still get a Property assignment expected JS validation error (hence bug #19629 that I just reported)

lizhiyao commented 7 years ago

I didn't disable JS validation.

vs code version: 1.8.1 vetur version: 0.3.0

in my settings.json:

{
    "files.associations": {
        "*.vue": "vue"
    }
}