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

[no-duplicates] additional new lines are generated when the list of imports to squash is long #2027

Closed
ertrzyiks opened this issue Apr 12, 2021 · 13 comments · Fixed by #2028
Closed

Comments

@ertrzyiks
Copy link
Contributor

ertrzyiks commented Apr 12, 2021

I stubled upon a weird problem. My eslint --fix results in excessive spacing created, a few additional new lines are inserted with no reason.

I narrowed down the case to the clash of those 3 rules:

  • no-multiple-empty-lines
  • import/no-duplicates
  • import/order

The original file:

import { One } from '../fragments/one'
import { Two } from '../fragments/two'
import { Three } from '../fragments/three'
import { Four } from '../fragments/four'
import { Five } from '../fragments/five'
import { Six } from '../fragments/six'
import { Seven } from '../fragments/seven'
import { Eight } from '../fragments/eight'
import { TypedDocumentNode as DocumentNode } from '@graphql-typed-document-node/core'
import { OneDoc } from '../fragments/one'
import { TwoDoc } from '../fragments/two'
import { ThreeDoc } from '../fragments/three'
import { FourDoc } from '../fragments/four'
import { FiveDoc } from '../fragments/five'
import { SixDoc } from '../fragments/six'
import { SevenDoc } from '../fragments/seven'
import { EightDoc } from '../fragments/eight'
export const Vars = any

The result of eslint --fix

import { TypedDocumentNode as DocumentNode } from '@graphql-typed-document-node/core'

import { One , OneDoc } from '../fragments/one'
import { Two , TwoDoc } from '../fragments/two'
import { Three , ThreeDoc } from '../fragments/three'
import { Four , FourDoc } from '../fragments/four'
import { Five , FiveDoc } from '../fragments/five'
import { Six , SixDoc } from '../fragments/six'
import { Seven , SevenDoc } from '../fragments/seven'
import { Eight , EightDoc } from '../fragments/eight'








export const Vars = any

The final configuration looks like below

{
  "plugins": ["import"],
  "parserOptions": {
    "sourceType": "module",
    "ecmaVersion": 2015
  },
  "rules": {
    "import/no-duplicates": ["error"],
    "import/order": [
      "error",
      {
        "groups": [
          [
            "builtin",
            "external"
          ],
          "internal",
          [
            "parent",
            "sibling",
            "index"
          ]
        ],
        "newlines-between": "always"
      }
    ],
    "no-multiple-empty-lines": ["error", {
      "max": 1,
      "maxEOF": 0
    }]
  }
}

I put together an example repo affected by the issue: https://github.com/ertrzyiks/import-order-multiple-empty-lines-demo

The result of eslint --fix is the following error:

12:1  error  More than 1 blank line not allowed  no-multiple-empty-lines

Examples

8 imports

A file with 8 imports to squash looks good 👍

https://github.com/ertrzyiks/import-order-multiple-empty-lines-demo/blob/main/example8.js
https://github.com/ertrzyiks/import-order-multiple-empty-lines-demo/blob/main/results/example8.js

9 imports

A file with 9 imports starts to generate empty lines at the end of the imports block

https://github.com/ertrzyiks/import-order-multiple-empty-lines-demo/blob/main/example9.js
https://github.com/ertrzyiks/import-order-multiple-empty-lines-demo/blob/main/results/example9.js

10 imports

A file with 10 imports starts to generate empty lines between the imports block and left-overs from merging

https://github.com/ertrzyiks/import-order-multiple-empty-lines-demo/blob/main/example10.js
https://github.com/ertrzyiks/import-order-multiple-empty-lines-demo/blob/main/results/example10.js

@ljharb
Copy link
Member

ljharb commented Apr 12, 2021

And if you run eslint --fix again? Perhaps no-multiple-empty-lines isn't autofixable.

@ertrzyiks
Copy link
Contributor Author

@ljharb a subsequent run of eslint --fix collapses the new lines into a single occurrence and produces:

import { TypedDocumentNode as DocumentNode } from '@graphql-typed-document-node/core'

import { One , OneDoc } from '../fragments/one'
import { Two , TwoDoc } from '../fragments/two'
import { Three , ThreeDoc } from '../fragments/three'
import { Four , FourDoc } from '../fragments/four'
import { Five , FiveDoc } from '../fragments/five'
import { Six , SixDoc } from '../fragments/six'
import { Seven , SevenDoc } from '../fragments/seven'
import { Eight , EightDoc } from '../fragments/eight'

export const Vars = any

@ljharb
Copy link
Member

ljharb commented Apr 13, 2021

So what's the issue? eslint autofixing sometimes does require multiple passes.

@ertrzyiks
Copy link
Contributor Author

ertrzyiks commented Apr 13, 2021

The issue is that those extra new lines are generated. If we remove no-multiple-empty-lines rule from this setup, the linter is happy about the result. It's still unexpected to have so many new line characters injected.

eslint autofixing sometimes does require multiple passes.

Afaik eslint makes multiple runs of autofixes on its own, why do I need to re-run the command? Anyway, even if it happens that some rules require multiple runs I think it's a misbehavior and can be possibly fixed.

@ljharb
Copy link
Member

ljharb commented Apr 13, 2021

ahhh, i see what you mean.

If the autofixes from the import rules - when enabled alone - are producing those empty lines, then that does seem like something we can avoid doing.

@ertrzyiks
Copy link
Contributor Author

ertrzyiks commented Apr 13, 2021

Correct, the empty lines are the result of auto-fixing.

I plan to take a closer look what exactly is producing empty lines, at the moment I only narrowed it down to those 3 rules: semi, import/order and import/no-duplicates. When I disable one of those, the empty lines are not produced.

The case I provided is also important to the reproduction, when the example.js is slightly adjusted the problem is gone. It must be some really edge case.

@ertrzyiks ertrzyiks changed the title Conflict between order,no-duplicates,semi Conflict between order and no-duplicates Apr 13, 2021
@ertrzyiks
Copy link
Contributor Author

I eliminated the role of semi rule as the issue appears also without it enabled. We are left with order and no-duplicates. I updated the repo and the issue with more examples.

It appears that the new lines are injected starting from 9th import to squash if we have those two rules enabled. However, when I keep just no-duplicates rule, the problem starts to appear from 10th import to squash.

{
  "plugins": ["import"],
  "parserOptions": {
    "sourceType": "module",
    "ecmaVersion": 2015
  },
  "rules": {
    "import/no-duplicates": ["error"]
  }
}

The result is

import { One , OneDoc } from '../fragments/one'
import { Two , TwoDoc } from '../fragments/two'
import { Three , ThreeDoc } from '../fragments/three'
import { Four , FourDoc } from '../fragments/four'
import { Five , FiveDoc } from '../fragments/five'
import { Six , SixDoc } from '../fragments/six'
import { Seven , SevenDoc } from '../fragments/seven'
import { Eight , EightDoc } from '../fragments/eight'
import { Nine , NineDoc } from '../fragments/nine'
import { Ten , TenDoc } from '../fragments/ten'
import { TypedDocumentNode as DocumentNode } from '@graphql-typed-document-node/core'










export const Vars = any

@ljharb
Copy link
Member

ljharb commented Apr 13, 2021

Seems like no-duplicates is to blame.

@ertrzyiks ertrzyiks changed the title Conflict between order and no-duplicates [no-duplicates] additional new lines are generated when the list of imports to squash is long Apr 13, 2021
@ertrzyiks
Copy link
Contributor Author

ertrzyiks commented Apr 13, 2021

I agree and reflected that in the issue title. I'm looking into the problem but I'm new to the codebase so it may take a while until I grasp the idea behind the fixer. Any help/pointers are appreciated 🙇

@ljharb
Copy link
Member

ljharb commented Apr 13, 2021

Thanks! I'll take a look at #2028 and offer as much help as i can, once i have time to do so :-)

@ertrzyiks
Copy link
Contributor Author

I marked #2028 as ready for review. Turned out that this problem occurs with any number of imports. I guess it's not so popular to have more than 2 duplicated imports so it was not causing any issues for most of the users.

@ljharb
Copy link
Member

ljharb commented Apr 15, 2021

Also since it’s corrected by a second autofix, people may have not bothered reporting it.

@ertrzyiks
Copy link
Contributor Author

I have some trouble with failing tests, they highlighted some more scenarios not covered by my change. The test cases are formatted with leading spaces like

    import ...
    import 

so removing just a new line character produces a very weird formatting. I considered reworking the test to ignore the leading spaces, but IMO it shows that the fix tries to be too smart now.

The problem occurred in autogenerated files, it may be much easier to update the generator to not produce duplicates.

Any thoughts on this @ljharb ? I'm leaning towards closing this issue as works as intended.

ljharb pushed a commit to ertrzyiks/eslint-plugin-import that referenced this issue May 14, 2021
ljharb pushed a commit to ertrzyiks/eslint-plugin-import that referenced this issue May 14, 2021
ertrzyiks added a commit to ertrzyiks/eslint-plugin-import that referenced this issue May 16, 2021
ljharb pushed a commit to ertrzyiks/eslint-plugin-import that referenced this issue May 29, 2021
@ljharb ljharb closed this as completed in 7aea664 May 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

2 participants