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

object-curly-newline for named imports not working as expected #12018

Closed
antoniogiordano opened this issue Jul 24, 2019 · 16 comments
Closed

object-curly-newline for named imports not working as expected #12018

antoniogiordano opened this issue Jul 24, 2019 · 16 comments
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion auto closed The bot closed this issue works as intended The behavior described in this issue is working correctly

Comments

@antoniogiordano
Copy link

Tell us about your environment

  • ESLint Version:
    6.1.0
  • Node Version:
    12.2
  • npm Version:
    6.9.0

What parser (default, Babel-ESLint, etc.) are you using?
default through webstorm

Please show your full configuration:

Configuration
{
  "parser": "babel-eslint",
  "extends": [
    "airbnb"
  ],
  "env": {
    "browser": true
  },
  "rules": {
    "arrow-parens": "off",
    "comma-dangle": ["error", "never"],
    "semi": ["error", "never"],
    "no-confusing-arrow": "off",
    "no-unused-expressions": "off",
    "react/jsx-filename-extension": [1, { "extensions": [".js", ".jsx"] }],
    "react/jsx-one-expression-per-line": "off",
    "import/prefer-default-export": "off",
    "no-param-reassign": "off",
    "no-unused-vars": "off",
    "import/extensions": "off",
    "react/no-unused-prop-types": "off",
    "object-curly-newline": ["error", {
      "ObjectExpression": {"minProperties": 1},
      "ObjectPattern": {"minProperties": 1},
      "ImportDeclaration": {
        "multiline": true,
        "minProperties": 1,
        "consistent": true
      },
      "ExportDeclaration": "always"
    }],
    "object-property-newline": ["error", {
      "allowAllPropertiesOnSameLine": false
    }]
  },
  "settings": {
    "import/resolver": {
      "webpack": {
        "config": "configs/webpack.config.dev.js"
      }
    }
  },
  "settings": {
    "import/resolver": {
      "webpack": {
        "config": "configs/webpack.config.dev.js"
      }
    }
  },
  "plugins": [
    "react"
  ]
}

What did you do? Please include the actual source code causing the issue, as well as the command that you used to run ESLint.

automatically done by webstorm

What did you expect to happen?
I expect this code

import {
  Button, Divider,
  Grid, LinearProgress, Link, MuiThemeProvider, Paper, Typography, withStyles
} from '@material-ui/core'

What actually happened? Please include the actual, raw output from ESLint.
to be like this

import {
  Button, 
  Divider,
  Grid,
  LinearProgress, 
  Link, 
  MuiThemeProvider, 
  Paper, 
  Typography, 
  withStyles
} from '@material-ui/core'

Are you willing to submit a pull request to fix this bug?
No

@antoniogiordano antoniogiordano added bug ESLint is working incorrectly triage An ESLint team member will look at this issue soon labels Jul 24, 2019
@kaicataldo
Copy link
Member

It's not clear to me if this is reporting an issue with auto fixing or a lint error. Do you mind clarifying, and if it is a lint error, share the output of that error?

Additionally, to clarify whether it's an issue with ESLint itself or the Webstorm ESLint integration, can you run ESLint from the command line and confirm that it yields the same results?

@kaicataldo kaicataldo added evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion rule Relates to ESLint's core rules and removed triage An ESLint team member will look at this issue soon labels Jul 24, 2019
@antoniogiordano
Copy link
Author

Yes autofixing is enabled, even launching the command from the console there is no fix in the import statement.
The error is referred to the object-curly-newline rule, because it doesn't put all the import vars on new lines like I would expect

@mastacheata
Copy link

mastacheata commented Jul 24, 2019

I can probably expand a little on this, but only using the 5.16.0 version of ESLint and using babel-eslint as the parser. (limited to 5.16.0 as we're using create-react-app on the project and that has a hard requirement for the 5.x series of ESLint)
I'm experiencing the same problem as @antoniogiordano is, but I at least know how to provide additional information on my config.

This is the simplified rule from my .eslintrc.js:

    'object-curly-newline': ['error', {
        "ImportDeclaration": { "minProperties": 3, "consistent": false, "multiline": true },
    }],

According to the example on the ESLint documentation ( https://eslint.org/docs/rules/object-curly-newline#multiline) the following line should raise an error with ESLint as it has a) >= 3 named imports and b) all of them on the very same line, which should be forbidden by the multiline: true setting:

import {
  ButtonControl, DatePickerControl, SelectControl, TextboxControl,
} from '../../../../shared/Form/elements';

I would expect this to be reformatted to have each import on it's own line:

import {
  ButtonControl,
  DatePickerControl,
  SelectControl,
  TextboxControl,
} from '../../../../shared/Form/elements';

Calling eslint from the commandline to analyze just one file like this:
.\node_modules\.bin\eslint --ext=jsx ./project/index.js

I also tried changing the object-property-newline rule to disable the allowAllPropertiesOnSameLine option, but that doesn't seem to have any effect at all.

If I change the conflicting import to be just a single line it will complain, but only move the named imports to the format specified above on a single line with the parenthesis on separate lines.

@phakphumi
Copy link

Same issue, even @mastacheata can't fix mine.

The lint can't enforce the rule to separate line from destructuring import. Like

import {
  Col, Row
} from 'antd';

Above code pass lint easily no warning no fixing.
My rules:

    "object-curly-newline": ["error", { "ImportDeclaration": { "multiline": true, "minProperties": 2 }}],
    "object-property-newline": "error",

@mneumark
Copy link

Same here. On my project, we want to be able to see github blame for each named import. To do that we need developers to put each named import on a separate line. So the ability to enforce one named import per line would be amazing.

@eslint-deprecated eslint-deprecated bot added the auto closed The bot closed this issue label Aug 31, 2019
@eslint-deprecated
Copy link

Unfortunately, it looks like there wasn't enough interest from the team
or community to implement this change. While we wish we'd be able to
accommodate everyone's requests, we do need to prioritize. We've found
that issues failing to reach accepted status after 21 days tend to
never be accepted, and as such, we close those issues.
This doesn't mean the idea isn't interesting or useful, just that it's
not something the team can commit to.

Thanks for contributing to ESLint and we appreciate your understanding.

@mi-mazouz
Copy link

+1

@brkn
Copy link

brkn commented Nov 30, 2019

Is there any plan on fixing this?

@mastacheata
Copy link

Obviously not, The issue is closed, so noone will have it on their radar for things to fix.
Unless anyone affected by this problem has enough knowledge about ESLint internals to at least draft a PR, this is unlikely to ever be fixed.

@mdjermanovic
Copy link
Member

Reopening to clarify, as this doesn't look like a bug.

object-curly-newline rule enforces line breaks after { and before }, i.e. between { and the first property and between the last property and }.

object-curly-newline does work with named imports, and from the examples above it seems that it works as intended (please feel free to post again an example where it doesn't work well if I'm missing something). This rule does not enforce line breaks between properties inside {}.

The rule to enforce line breaks between properties is object-property-newline, but it works only with object literals. There is an ongoing discussion in #9259 on whether this rule should be enhanced to support destructuring patterns/imports/exports as well.

@mdjermanovic mdjermanovic reopened this Nov 30, 2019
@mdjermanovic mdjermanovic added works as intended The behavior described in this issue is working correctly and removed auto closed The bot closed this issue bug ESLint is working incorrectly evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion rule Relates to ESLint's core rules labels Nov 30, 2019
@mastacheata
Copy link

But isn't the Multiline option of that rule exactly for this situation?
Also, IIRC (not at a computer to double check right now) if you have more items on one line, ESLint complains and offers to fix it, but doesn't fix it...
At least to me,that is in fact a bug.

@mdjermanovic
Copy link
Member

But isn't the Multiline option of that rule exactly for this situation?

multiline option in object-curly-newline requires or disallows linebreaks after { and before } depending on whether the content inside is multiline or not. It doesn't enforce line breaks between properties/names inside {}. Line breaks between (or inside) properties/names are 'input' to determine what should be enforced on braces.

Also, IIRC (not at a computer to double check right now) if you have more items on one line, ESLint complains and offers to fix it, but doesn't fix it...
At least to me,that is in fact a bug.

Could you please post an example where it seems that the rule doesn't work as expected? That would be helpful to find a potential bug or clarify what's the responsibility of this rule.

@mastacheata
Copy link

I'll gladly provide an example on Monday afternoon when I'm in the office again.

@eslint-deprecated eslint-deprecated bot added the auto closed The bot closed this issue label Jan 1, 2020
@eslint-deprecated
Copy link

Unfortunately, it looks like there wasn't enough interest from the team
or community to implement this change. While we wish we'd be able to
accommodate everyone's requests, we do need to prioritize. We've found
that issues failing to reach accepted status after 21 days tend to
never be accepted, and as such, we close those issues.
This doesn't mean the idea isn't interesting or useful, just that it's
not something the team can commit to.

Thanks for contributing to ESLint and we appreciate your understanding.

@lukenofurther
Copy link
Contributor

@mdjermanovic if this isn't a bug then the documentation for the rule isn't accurate. According to the documentation:

// With this rule config in .eslintrc.js
"object-curly-newline": ["error", {
  "ImportDeclaration": "always",
}]

// operating on this subject statement
import { foo, bar } from "lodash"

// the rule's documentation implies that eslint --fix will change the subject to
import {
  foo,
  bar
} from "lodash"

// However, the actual result of eslint --fix is
import {
    foo, bar,
} from "lodash"

This is different from the implied "correct code" example in the rule's documentation.

The result of the ES Lint fix is still breaking AirBnB styleguide rule 10.8 so I'm surprised you won't fix it.

If you don't fix it then please at least update the documentation to reflect what --fix really does, and/or prevent this rule from being fixed for import statements at all.

@mdjermanovic
Copy link
Member

@lukenofurther thanks for the suggestions!

Correct examples do not intend to represent the result of eslint --fix applied to incorrect examples.

"object-curly-newline": ["error", { "ImportDeclaration": "always" }] expects a line break after {, and a line break before }.

Any code that satisfies these conditions is correct for this rule:

// correct example
import {
  foo,
  bar
} from "lodash"

// also correct example
import {
    foo, bar
} from "lodash"

// incorrect example
import { foo, bar } from "lodash"

// incorrect example
import { foo, 
bar } from "lodash"

// incorrect example
import { foo, bar
} from "lodash"

// incorrect example
import { 
foo, 
bar } from "lodash"

The documentation is correct in this case, but it would be nice to have both examples (with and without line breaks between foo and bar). PR is welcome!

--fix works as intended, because it makes a minimal change in the code to make it valid for this rule.

As for the line breaks between foo and bar, we don't have a rule to enforce/disallow these at the moment. Feel free to open a new rule proposal!

@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Jul 1, 2020
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Jul 1, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion auto closed The bot closed this issue works as intended The behavior described in this issue is working correctly
Projects
None yet
Development

No branches or pull requests

9 participants