xojs/xo

Unable to customize certain rules via shareable config #71

vadimdemedes posted onGitHub

Using latest xo (master branch), I'm unable to customize babel/object-curly-spacing rule via shareable config.

package.json:

{
  "extends": "my-config"
}

my-config:

{
  "rules": {
    "babel/object-curly-spacing": 0
  }
}

file.js:

{ key: 'value' }

Given these files, xo throws error:

<img width="570" alt="screen shot 2016-01-08 at 10 18 49 pm" src="https://cloud.githubusercontent.com/assets/697676/12208509/d5c9078e-b655-11e5-9373-0ff60e116789.png">

If I override babel/object-curly-spacing in package.json, it works as expected and I no longer get errors:

{
  "rules": {
    "babel/object-curly-spacing": 0
  }
}

Related: https://github.com/sindresorhus/xo/blob/master/options-manager.js#L98


Related: https://github.com/sindresorhus/xo/blob/2d26d79ea81f893fc7b1ff9e586c1e74ad28c830/options-manager.js#L98

Hmm. We just set the default value there. It's later extended by your custom config.

posted by sindresorhus about 9 years ago

It's later extended by your custom config.

Seems like it does not for some reason. Needs investigation.

posted by vadimdemedes about 9 years ago

Those rules gets set in the root of the options sent in to eslint and will therefore take precedence over any configs. Configs will only override other configs (I think). The rules referenced in https://github.com/sindresorhus/xo/blob/2d26d79ea81f893fc7b1ff9e586c1e74ad28c830/options-manager.js#L98 are only overridable through setting rules in the package.json, or wherever your config lives.

posted by kevva about 9 years ago
posted by vadimdemedes about 9 years ago

What would be the consequence of moving those defaults into another base config that we conditionally add to extends, instead of force setting them like that?

posted by jamestalmage about 9 years ago

I don't think there are any consequences other than creating a config for it may seem unnecessary but in this case it might be needed.

posted by kevva about 9 years ago

Why are those rules not in the default xo (or xo-react) config in the first place?

posted by vadimdemedes about 9 years ago

Why does xo forces these default rules here (https://github.com/sindresorhus/xo/blob/2d26d79ea81f893fc7b1ff9e586c1e74ad28c830/options-manager.js#L98) ?

The Babel parser and plugin have some incompatible rules. So we first need to reset the original rules, then set the Babel ones. That line just sets it to the same value as in eslint-config-xo.

What would be the consequence of moving those defaults into another base config that we conditionally add to extends, instead of force setting them like that?

That would be the way to go. Anyone wanna do a PR?

Why are those rules not in the default xo (or xo-react) config in the first place?

They are, but here we do something different as we're using the esnext rules even though the esnext option is not set. We do this so esnext features will still work without the esnext option, just not linted as such.

posted by sindresorhus about 9 years ago

This should be fixed in https://github.com/sindresorhus/xo/commit/cece3dd2893efe0499380e6f8db654c18b6210e6, but with ESLint changes and this, it would be nice if you all could take a look at the changes.

@vdemedes Can you try it out in practice?

posted by sindresorhus about 9 years ago

Tried it out in a real app and now shareable configs don't work at all:

$ npm install eslint-config-vdemedes

package.json:

"xo": {
  "extends": ["vdemedes"]
}

output when running latest xo:

Error: Cannot find module 'eslint-config-vdemedes' from '/Users/vdemedes/Projects/xo/node_modules'
Referenced from:
    at Function.module.exports [as sync] (/Users/vdemedes/Projects/xo/node_modules/resolve/lib/sync.js:33:11)
    at resolve (/Users/vdemedes/Projects/xo/node_modules/eslint/lib/config/config-file.js:417:38)
    at load (/Users/vdemedes/Projects/xo/node_modules/eslint/lib/config/config-file.js:436:24)
    at /Users/vdemedes/Projects/xo/node_modules/eslint/lib/config/config-file.js:355:36
    at Array.reduceRight (native)
    at Object.applyExtends (/Users/vdemedes/Projects/xo/node_modules/eslint/lib/config/config-file.js:338:28)
    at loadConfig (/Users/vdemedes/Projects/xo/node_modules/eslint/lib/config.js:63:37)
    at new Config (/Users/vdemedes/Projects/xo/node_modules/eslint/lib/config.js:170:44)
    at CLIEngine.executeOnFiles (/Users/vdemedes/Projects/xo/node_modules/eslint/lib/cli-engine.js:492:28)
    at runEslint (/Users/vdemedes/Projects/xo/index.js:67:16)

Might be related: https://github.com/eslint/eslint/issues/5411.

posted by vadimdemedes about 9 years ago

Added a PR to demonstrate this issue - #83.

posted by vadimdemedes about 9 years ago

I ended up reverting to the previous workaround. Don't have time to fix this properly right now. New version is out btw. I still would like to see this fixed at some point.

posted by sindresorhus about 9 years ago

I ended up reverting to the previous workaround

Are you saying that there is another way to have different configs for subfolders?

posted by bfred-it about 8 years ago

@bfred-it I'm not sure what you're asking? Doesn't seem relevant to this thread.

posted by sindresorhus about 8 years ago

Sorry, I was referring to the overrides property. From what I understand it was disabled and it was dependent on this issue (as you said here https://github.com/sindresorhus/xo/issues/78#issuecomment-182350712)

I thought you were referring to overrides in "I ended up reverting to the previous workaround (for overrides?)"

posted by bfred-it about 8 years ago

@bfred-it I don't remember, but it ended up landing regardless: https://github.com/sindresorhus/xo#config-overrides

posted by sindresorhus almost 8 years ago

@issuehunt has funded $60.00 to this issue.


posted by IssueHuntBot almost 6 years ago

This will be fixed with #702

posted by sindresorhus 8 months ago

The bounty has been moved to #702

posted by sindresorhus 8 months ago

Fund this Issue

$60.00
Funded

Pull requests

Recent activities

issuehunt funded 60.00 for xojs/xo# 71
almost 6 years ago