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

[New] add named-order rule to sort specifiers of import, export, require #2552

Closed
wants to merge 24 commits into from
Closed

[New] add named-order rule to sort specifiers of import, export, require #2552

wants to merge 24 commits into from

Conversation

ronparkdev
Copy link

@ronparkdev ronparkdev commented Sep 18, 2022

Examples

✅ Valid with named-order

import { a, b } from './foo';
const { a, b } = require('./foo');
export { a, b };

❌ Invalid with named-order

import { b, a } from './foo';
const { b, a } = require('./foo');
export { b, a };

Why?

I experienced a significant decrease in readability as the number of specifiers increased in the named import. So, I made this rule. Tell me if there's anything weird in this rule 😄

@ronparkdev ronparkdev changed the title [New] add named-order rule to sort specifiers of import [New] add named-order rule to sort specifiers of import, export, require Sep 18, 2022
@ljharb
Copy link
Member

ljharb commented Sep 18, 2022

Please discuss things in issues before sending a PR.

What’s your use case where you have that many named imports? Things should be deep-imported from granular modules.

import { alpha, Alpha, ALPHA } from 'foo' // <- will not report
```

#### `order`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks to you, I realized the weirdness in some cases. I think it should consider the options below.

  • "case-insensitive': Correct order is "Bar", "baz", "Foo". (This is the default.)
  • "lowercase-first": Correct order is "baz", "Bar", "Foo".
  • "lowercase-last": Correct order is "Bar", "Foo", "baz".

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JounQin, Applied new order option to here 41b5888
thank you :)

@ronparkdev
Copy link
Author

Please discuss things in issues before sending a PR.

What’s your use case where you have that many named imports? Things should be deep-imported from granular modules.

Oh I see, I will write an issue about this. So Is it ok to change to draft, or should be closed this PR?

@ronparkdev ronparkdev marked this pull request as draft September 18, 2022 14:08
@JounQin
Copy link
Collaborator

JounQin commented Sep 18, 2022

What’s your use case where you have that many named imports? Things should be deep-imported from granular modules.

It is a good to have feature when using something like rxjs IMO:

import {
  BehaviorSubject,
  combineLatest,
  distinctUntilChanged,
  map,
  switchMap,
  tap,
} from 'rxjs';

It will reduce conflicts when collaborating with others.

@ronparkdev
Copy link
Author

I wrote an issue for discussion about this 😄
#2553

@yangirov
Copy link

yangirov commented Mar 3, 2023

@ronparkdev hi, if we have an import like import fetch, { RequestInit, Response } from 'node-fetch' then your rule gives such an error. 🥲

CodeSandbox

TypeError: Cannot read properties of undefined (reading 'name')
Occurred while linting /project/sandbox/src/server.ts:2
    at /project/sandbox/node_modules/eslint-plugin-eslint-custom-rules/rules/named-order.js:53:49
    at Array.reduce (<anonymous>)
    at getNormalizedValue (/project/sandbox/node_modules/eslint-plugin-eslint-custom-rules/rules/named-order.js:53:21)
    at sorter (/project/sandbox/node_modules/eslint-plugin-eslint-custom-rules/rules/named-order.js:70:24)
    at Array.sort (<anonymous>)
    at handleImports (/project/sandbox/node_modules/eslint-plugin-eslint-custom-rules/rules/named-order.js:130:50)
    at /project/sandbox/node_modules/eslint/lib/linter/safe-emitter.js:45:58
    at Array.forEach (<anonymous>)
    at Object.emit (/project/sandbox/node_modules/eslint/lib/linter/safe-emitter.js:45:38)
    at NodeEventGenerator.applySelector (/project/sandbox/node_modules/eslint/lib/linter/node-event-generator.js:293:26)
error Command failed with exit code 2.

@ronparkdev ronparkdev closed this by deleting the head repository Jan 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants