Bug with object spread and computed-property-even-spacing #3

Closed
cesarandreu opened this Issue Aug 4, 2015 · 21 comments

Projects

None yet

6 participants

@cesarandreu

The following two examples are giving me errors:

// First
const { a, ...b } = obj

// Second
func(a, { ...b })
standard: Use JavaScript Standard Style (https://github.com/feross/standard)
  <text>:44:22: Expected 1 or 0 spaces around "[" and "]" (standard/computed-property-even-spacing)
  <text>:72:25: Expected 1 or 0 spaces around "[" and "]" (standard/computed-property-even-spacing) 

Bug or intentional?

Using babel-eslint@4.0.5 and standard@5.0.0.

@feross
Collaborator
feross commented Aug 4, 2015

Looks like a bug. That code should work. @xjamundx - any ideas?

@xjamundx
Owner
xjamundx commented Aug 4, 2015

Okay, so we need to update the rule to take into account object-spread, which just came out. I'll take a stab at it today! The other one seems like a bug. Will address both.

@xjamundx
Owner
xjamundx commented Aug 4, 2015

So I can't reproduce this with raw ESLint using the plugins tests, which means it could be a number of things.

  1. babel-eslint could be the culprit. It's not needed for object rest/spread, so you may not need it.
  2. standard itself may not be using the latest eslint or maybe it's not enabling experimentalObjectRestSpread: true

When I run standard 5.0.0 on the file with your code examples I get the following:

standard: Use JavaScript Standard Style (https://github.com/feross/standard)
  /Users/jamuferguson/dev/eslint-plugin-standard/test.js:1:13: Unexpected token ...

I will send up a PR proving the plugin is capable of handling this and @feross can you see if it's a standard issue or something else?

@xjamundx xjamundx pushed a commit that referenced this issue Aug 4, 2015
Jamund Ferguson Tests: obj/rest spread and computed-property rule (fixes #3) ecd8b8d
@feross feross pushed a commit that closed this issue Aug 6, 2015
Jamund Ferguson Tests: obj/rest spread and computed-property rule (fixes #3) ecd8b8d
@feross feross closed this in ecd8b8d Aug 6, 2015
@feross feross reopened this Aug 6, 2015
@feross
Collaborator
feross commented Aug 6, 2015

I'm not sure why standard is giving an "unexpected token ..." error. It happens even when I enable experimentalObjectRestSpread. And standard is using eslint 1.0.0.

Not sure what's going on here. I'm not very motivated to fix this since it's an experimental, likely-to-change language feature. But if there's a simple PR to fix this, I'm happy to merge it.

@xjamundx
Owner
xjamundx commented Aug 6, 2015

Possibly related:
eslint/eslint#2346 (comment)

@dignifiedquire

Seeing this as well with babel-eslint + eslint-config-standard.

npm ls eslint eslint-config-standard babel-eslint

├── babel-eslint@4.0.5
├── eslint@1.0.0
├── eslint-config-standard@4.0.0
├─┬ grunt-eslint@17.0.0
│ └── eslint@1.0.0
@xjamundx
Owner
xjamundx commented Aug 6, 2015

It's quite possibly a babel-eslint issue. I'll do a bit more research though on both standard and babel-eslint

@jprichardson
Collaborator

I'm having the same problem feross/standard#226. Here's exactly how to reproduce:

(in a new directory)

touch app.js

put the following content in app.js

import fn from 'fn'

export default function something (args) {
  return fn({
    field: 'blah',
    ...args
  })
}

install standard 4 and babel-eslint (to prove it's not babel-eslint):

npm i --save-dev standard@4.x
npm i --save-dev babel-eslint@4.0.5

create package.json (so we can use babel-eslint parser in standard):

touch package.json

add parser to package.json:

{
  "standard": {
    "parser": "babel-eslint"
  }
}

run standard to verify no errors:

./node_modules/.bin/standard --verbose

now upgrade to standard 5 to verify error:

npm i --save-dev standard@5.0.2

run standard again to verify error:

./node_modules/.bin/standard --verbose

Error received:

/private/tmp/standard-1/app.js:6:5: Expected "[" and "]" to be on the same line (standard/computed-property-even-spacing)

So I don't think it's babel-eslint given that it worked fine with standard 4, unless I'm missing something??

@xjamundx
Owner

For clarity this is 99% likely a babel-eslint issue, but I will find time today to confirm.

@xjamundx
Owner

okay looking into this in-depth today

@xjamundx
Owner

Related: yannickcr/eslint-plugin-react#187

So @feross if you want this to work in standard by default you need to do the following:

  1. Update to the latest eslint-plugin-react (preferably after that patch lands)
  2. Update to the latest eslint (1.1.0)
  3. Add experimentalObjectRestSpread: true here https://github.com/feross/eslint-config-standard/blob/master/eslintrc.json#L3 (because it's not an es6 feature you don't get it by default) feross/eslint-config-standard#10

Once you do that the code above should run fine with standard and any other issues are very likely to do with babel-eslint trying to account for object rest/spread. @hzoo

@hzoo
hzoo commented Aug 11, 2015

Something we didn't since object spread wasn't standard yet - https://github.com/babel/babel-eslint/blob/85e3f82ec096499895cc12ab8955bf63b216324f/acorn-to-esprima.js#L178-L191- I would have to look at the rule to understand why it's failing

Oh - so it's checking for computed properties and babel-eslint made node.computed = true?

Maybe we don't need to make some of these changes @sebmck?

@feross
Collaborator
feross commented Aug 12, 2015

@xjamundx Nice work debugging this. I'll update once eslint-plugin-react is updated.

@feross
Collaborator
feross commented Aug 14, 2015

This is fixed in standard 5.1.0.

@feross feross closed this Aug 14, 2015
@cesarandreu

@feross Still getting an error:

// works
const {a, ...b} = c

// works
const { a } = b

// doesn't work
const { a, ...b } = c

Expected 1 or 0 spaces around "[" and "]" (standard/computed-property-even-spacing)

@xjamundx
Owner

haha. it never ends. i'll check it out in a bit.

@hzoo
hzoo commented Aug 14, 2015

this is with babel-eslint right? I'l probably have to make a change to fix this

@hzoo hzoo referenced this issue in babel/babel-eslint Aug 15, 2015
Merged

use SpreadProperty type, revert other transforms #165

@hzoo hzoo added a commit to hzoo/babel-eslint that referenced this issue Aug 15, 2015
@hzoo hzoo use SpreadProperty type, revert other transforms d75bf7e
@hzoo
hzoo commented Aug 15, 2015

Verified the fix - PR is babel/babel-eslint#165

@hzoo hzoo added a commit to hzoo/babel-eslint that referenced this issue Aug 16, 2015
@hzoo hzoo use SpreadProperty type, revert other transforms 30398ed
@hzoo
hzoo commented Aug 17, 2015

Ok should be fixed in babel-eslint 4.0.10

@chibicode chibicode referenced this issue in ricardofbarros/linter-js-standard Aug 18, 2015
Merged

Update babel-eslint 4.0.10, standard 5.1.0 #43

@hzoo
hzoo commented Aug 19, 2015

@cesarandreu let me know if it still fails

@cesarandreu

It's working properly now, thanks @hzoo :D.

@seniorquico seniorquico referenced this issue in ricardofbarros/linter-js-standard Sep 4, 2015
Merged

:arrow_up: babel-eslint 4.1.1, standard 5.2.1 #50

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment