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

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

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

Comments

6 participants
@cesarandreu
Copy link

commented Aug 4, 2015

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

This comment has been minimized.

Copy link
Member

commented Aug 4, 2015

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

@xjamundx

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor

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?

@feross feross closed this in ecd8b8d Aug 6, 2015

feross added a commit that referenced this issue Aug 6, 2015

Merge pull request #4 from xjamundx/tests-could-pass
Tests: obj/rest spread and computed-property rule (fixes #3)

@feross feross reopened this Aug 6, 2015

@feross

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2015

Possibly related:
eslint/eslint#2346 (comment)

@dignifiedquire

This comment has been minimized.

Copy link

commented Aug 6, 2015

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Member

commented Aug 8, 2015

I'm having the same problem standard/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

This comment has been minimized.

Copy link
Contributor

commented Aug 10, 2015

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

@xjamundx

This comment has been minimized.

Copy link
Contributor

commented Aug 11, 2015

okay looking into this in-depth today

@xjamundx

This comment has been minimized.

Copy link
Contributor

commented Aug 11, 2015

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) standard/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

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link
Member

commented Aug 12, 2015

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

@feross

This comment has been minimized.

Copy link
Member

commented Aug 14, 2015

This is fixed in standard 5.1.0.

@feross feross closed this Aug 14, 2015

@cesarandreu

This comment has been minimized.

Copy link
Author

commented Aug 14, 2015

@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

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2015

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

@hzoo

This comment has been minimized.

Copy link

commented Aug 14, 2015

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

@hzoo

This comment has been minimized.

Copy link

commented Aug 15, 2015

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

hzoo added a commit to hzoo/babel-eslint that referenced this issue Aug 16, 2015

@hzoo

This comment has been minimized.

Copy link

commented Aug 17, 2015

Ok should be fixed in babel-eslint 4.0.10

@hzoo

This comment has been minimized.

Copy link

commented Aug 19, 2015

@cesarandreu let me know if it still fails

@cesarandreu

This comment has been minimized.

Copy link
Author

commented Aug 19, 2015

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.