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

Warning: Object.keys called on non-object #1760

Closed
bierik opened this Issue Feb 3, 2015 · 12 comments

Comments

Projects
None yet
4 participants
@bierik

bierik commented Feb 3, 2015

Hi there.

I got the following error when i was trying to lint with the grunt-eslint plugin.

TypeError: Object.keys called on non-object
    at Function.keys (native)
    at mergeConfigs (/Users/kbieri/bricoler/simplelayout/node_modules/grunt-eslint/node_modules/eslint/lib/util.js:35:12)
    at /Users/kbieri/bricoler/simplelayout/node_modules/grunt-eslint/node_modules/eslint/lib/util.js:60:25
    at Array.forEach (native)
    at mergeConfigs (/Users/kbieri/bricoler/simplelayout/node_modules/grunt-eslint/node_modules/eslint/lib/util.js:35:25)
    at /Users/kbieri/bricoler/simplelayout/node_modules/grunt-eslint/node_modules/eslint/lib/util.js:60:25
    at Array.forEach (native)
    at Object.mergeConfigs (/Users/kbieri/bricoler/simplelayout/node_modules/grunt-eslint/node_modules/eslint/lib/util.js:35:25)
    at Config.getConfig (/Users/kbieri/bricoler/simplelayout/node_modules/grunt-eslint/node_modules/eslint/lib/config.js:282:19)
    at processFile (/Users/kbieri/bricoler/simplelayout/node_modules/grunt-eslint/node_modules/eslint/lib/cli-engine.js:131:31)

According to sindresorhus/grunt-eslint#47 it's not an issue in the plugin.

Thanks for help.

@nzakas nzakas added the triage label Feb 3, 2015

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Feb 3, 2015

Member

Can you share your ESLint configuration?

Member

nzakas commented Feb 3, 2015

Can you share your ESLint configuration?

@bierik

This comment has been minimized.

Show comment
Hide comment
@bierik

bierik Feb 4, 2015

Sorry guys I used a rule in a wrong way for escaping quotes.

bierik commented Feb 4, 2015

Sorry guys I used a rule in a wrong way for escaping quotes.

@bierik bierik closed this Feb 4, 2015

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Feb 4, 2015

Member

That's okay, can you share which rule? Even if you misconfigure, ESLint shouldn't throw an error.

Member

nzakas commented Feb 4, 2015

That's okay, can you share which rule? Even if you misconfigure, ESLint shouldn't throw an error.

@ot-brett-jones

This comment has been minimized.

Show comment
Hide comment
@ot-brett-jones

ot-brett-jones Feb 27, 2015

I got this error because of invalid json. I forgot to provide a value for the last line "no-mixed-spaces-and-tabs". Here is what I had in my .eslintrc.

{
  "env": {
    "browser": true,
    "node": true
  },
  "globals": {
    "angular": true
  },
  "rules": {
      "no-use-before-define": "nofunc",
      "strict": false,
      "eol-last": false,
      "no-underscore-dangle": false,
      "quotes": [1, 'double'],
      "no-space-before-semi": [1],
      "no-dangle-comma": [0],
      "indent": 4,
      "no-mixed-spaces-and-tabs"
  }
}

ot-brett-jones commented Feb 27, 2015

I got this error because of invalid json. I forgot to provide a value for the last line "no-mixed-spaces-and-tabs". Here is what I had in my .eslintrc.

{
  "env": {
    "browser": true,
    "node": true
  },
  "globals": {
    "angular": true
  },
  "rules": {
      "no-use-before-define": "nofunc",
      "strict": false,
      "eol-last": false,
      "no-underscore-dangle": false,
      "quotes": [1, 'double'],
      "no-space-before-semi": [1],
      "no-dangle-comma": [0],
      "indent": 4,
      "no-mixed-spaces-and-tabs"
  }
}
@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Feb 27, 2015

Member

Oh very interesting! We should at least be able to tell you that's the problem when it happens.

Member

nzakas commented Feb 27, 2015

Oh very interesting! We should at least be able to tell you that's the problem when it happens.

@nzakas nzakas reopened this Feb 27, 2015

@nzakas nzakas added enhancement core accepted and removed triage labels Feb 27, 2015

@glenjamin

This comment has been minimized.

Show comment
Hide comment
@glenjamin

glenjamin Mar 1, 2015

Contributor

picking this up

Contributor

glenjamin commented Mar 1, 2015

picking this up

glenjamin added a commit to glenjamin/eslint that referenced this issue Mar 1, 2015

@glenjamin

This comment has been minimized.

Show comment
Hide comment
@glenjamin

glenjamin Mar 1, 2015

Contributor

This occurred because the file shown above is valid YAML, and the value of that rule key becomes null.

yaml.safeLoad(require('fs').readFileSync('bust.json','utf8'))
{ env: { browser: true, node: true },
  globals: { angular: true },
  rules:
   { 'no-use-before-define': 'nofunc',
     strict: false,
     'eol-last': false,
     'no-underscore-dangle': false,
     quotes: [ 1, 'double' ],
     'no-space-before-semi': [ 1 ],
     'no-dangle-comma': [ 0 ],
     indent: 4,
     'no-mixed-spaces-and-tabs': null } }
Contributor

glenjamin commented Mar 1, 2015

This occurred because the file shown above is valid YAML, and the value of that rule key becomes null.

yaml.safeLoad(require('fs').readFileSync('bust.json','utf8'))
{ env: { browser: true, node: true },
  globals: { angular: true },
  rules:
   { 'no-use-before-define': 'nofunc',
     strict: false,
     'eol-last': false,
     'no-underscore-dangle': false,
     quotes: [ 1, 'double' ],
     'no-space-before-semi': [ 1 ],
     'no-dangle-comma': [ 0 ],
     indent: 4,
     'no-mixed-spaces-and-tabs': null } }
@glenjamin

This comment has been minimized.

Show comment
Hide comment
@glenjamin

glenjamin Mar 1, 2015

Contributor

I've pushed a PR that treats a null value as if the key wasn't there.

I could probably make it error on null, but that would likely be a larger change as currently there isn't any schema checking of config files before merging them together - and knowing this is an error relies on knowing valid values for this particular key.

Contributor

glenjamin commented Mar 1, 2015

I've pushed a PR that treats a null value as if the key wasn't there.

I could probably make it error on null, but that would likely be a larger change as currently there isn't any schema checking of config files before merging them together - and knowing this is an error relies on knowing valid values for this particular key.

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Mar 2, 2015

Member

I don't think we should quietly fail in this case because the error might never be found. I'd just throw an error saying "Invalid configuration for rule-name". That at least warns that there's a problem to fix.

Member

nzakas commented Mar 2, 2015

I don't think we should quietly fail in this case because the error might never be found. I'd just throw an error saying "Invalid configuration for rule-name". That at least warns that there's a problem to fix.

@glenjamin

This comment has been minimized.

Show comment
Hide comment
@glenjamin

glenjamin Mar 2, 2015

Contributor

@michaelficarra At the moment there's no validation on rules at all, there's already #967 which dicusses why this is quite an involved change.

@nzakas would the better fix to add some basic config validation on each config file, that runs before merging, and that disallows null as a rule config value?

Contributor

glenjamin commented Mar 2, 2015

@michaelficarra At the moment there's no validation on rules at all, there's already #967 which dicusses why this is quite an involved change.

@nzakas would the better fix to add some basic config validation on each config file, that runs before merging, and that disallows null as a rule config value?

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Mar 2, 2015

Member

That's a much larger effort, which is why it hasn't been done yet (see #967). The easiest path forward here is to throw an error.

Member

nzakas commented Mar 2, 2015

That's a much larger effort, which is why it hasn't been done yet (see #967). The easiest path forward here is to throw an error.

glenjamin pushed a commit to glenjamin/eslint that referenced this issue Mar 2, 2015

@glenjamin

This comment has been minimized.

Show comment
Hide comment
@glenjamin

glenjamin Mar 2, 2015

Contributor

Have pushed another draft - the logic has to live in config.js rather than mergeConfig in util.js as there isn't enough context to detect the error otherwise. Unless we say that null is never a valid value in a config file anywhere.

Contributor

glenjamin commented Mar 2, 2015

Have pushed another draft - the logic has to live in config.js rather than mergeConfig in util.js as there isn't enough context to detect the error otherwise. Unless we say that null is never a valid value in a config file anywhere.

@nzakas nzakas closed this in 412c9c7 Mar 6, 2015

nzakas added a commit that referenced this issue Mar 6, 2015

Merge pull request #1912 from glenjamin/handle-null-rule-config
Fix: don't crash when given null as rule config (fixes #1760)

@eslint eslint bot locked and limited conversation to collaborators Feb 7, 2018

@eslint eslint bot added the archived due to age label Feb 7, 2018

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