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

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

Closed
garthk opened this issue Mar 8, 2016 · 35 comments
Closed

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

garthk opened this issue Mar 8, 2016 · 35 comments
Assignees
Labels
javascript JavaScript support issues upstream Issue identified as 'upstream' component related (exists outside of VS Code)

Comments

@garthk
Copy link

garthk commented Mar 8, 2016

… 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:

  • Property assignment expected.
  • Argument expression expected.
  • Declaration or statement expected.

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

@kumarharsh
Copy link
Contributor

+1

@dlong500
Copy link

dlong500 commented Mar 8, 2016

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
Copy link
Member

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
Copy link
Member

egamma commented Mar 9, 2016

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

@egamma
Copy link
Member

egamma commented Mar 9, 2016

We will revive the javascript.validate.enable setting #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
Copy link
Contributor

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
Copy link
Member

egamma commented Mar 9, 2016

@kumarharsh this explanation makes sense, thanks.

@josebalius
Copy link
Member

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

@dlong500
Copy link

dlong500 commented Mar 9, 2016

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
Copy link
Author

garthk commented Mar 10, 2016

@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
Copy link
Member

egamma commented Mar 10, 2016

@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
Copy link
Member

egamma commented Mar 10, 2016

@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
Copy link

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
Copy link
Member

egamma commented Mar 10, 2016

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 microsoft/TypeScript#2103 (comment)

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
Copy link

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

@egamma
Copy link
Member

egamma commented Mar 15, 2016

@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
Copy link
Member

egamma commented Mar 16, 2016

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

@kumarharsh
Copy link
Contributor

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

@egamma
Copy link
Member

egamma commented Mar 17, 2016

@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
Copy link
Contributor

@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
Copy link
Member

egamma commented Mar 17, 2016

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
Copy link
Author

garthk commented Mar 21, 2016

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.

@egamma egamma added the upstream Issue identified as 'upstream' component related (exists outside of VS Code) label Apr 17, 2016
@egamma egamma added the javascript JavaScript support issues label Apr 17, 2016
@frogcjn
Copy link

frogcjn commented Jun 8, 2016

VSCode version 1.2.0 still has this problem?

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

[js] Property assignment expected.

@kumarharsh
Copy link
Contributor

@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
Copy link

frogcjn commented Jun 9, 2016

@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
Copy link
Contributor

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

@frogcjn
Copy link

frogcjn commented Jun 9, 2016

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

@kumarharsh
Copy link
Contributor

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.

alloy added a commit to artsy/metaphysics that referenced this issue Nov 13, 2016
This is to support the use of the spread operator where ever we include IDFields.
microsoft/vscode#3804
mzikherman pushed a commit to artsy/metaphysics that referenced this issue Nov 13, 2016
* [Artist] Only include displayable shows.

Fixes #473

* [vscode] Disable TypeScript based validation in favour of ESLint.

This is to support the use of the spread operator where ever we include IDFields.
microsoft/vscode#3804

* [Artist] Share common implementations of partner_shows and shows fields.

* [Artist] Revert addition of only displayable shows filter.

* [PartnerShow] Guard against partner data missing.
@waderyan waderyan assigned waderyan and unassigned egamma Nov 21, 2016
@mjbvz
Copy link
Collaborator

mjbvz commented Dec 6, 2016

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

@mjbvz mjbvz closed this as completed Dec 6, 2016
@lizhiyao
Copy link

lizhiyao commented Jan 16, 2017

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

.vue files

@zfeher
Copy link

zfeher commented Jan 25, 2017

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
Copy link

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

@lizhiyao
Copy link

lizhiyao commented Jan 31, 2017

My solution is vetur.

@ronjouch
Copy link

ronjouch commented Jan 31, 2017

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
Copy link

lizhiyao commented Feb 3, 2017

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"
    }
}

@vscodebot vscodebot bot locked and limited conversation to collaborators Nov 18, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
javascript JavaScript support issues upstream Issue identified as 'upstream' component related (exists outside of VS Code)
Projects
None yet
Development

No branches or pull requests