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 Rule suggestion: sort-dependencies #3143

Closed
madeiras opened this Issue Jul 23, 2015 · 22 comments

Comments

Projects
None yet
@madeiras
Copy link

commented Jul 23, 2015

Acronyms

TBD - to be discussed

The use case for the rule - what is it trying to prevent or flag? Include code examples.

This rule would flag unordered dependencies preferably configurable.

Env: Node

Default flagging: Symbols -> Upper case letters -> Lower case letters

Examples flagged

es6

import * as lib from './foo';
import { x, z , F } from './bar';
import _ from 'underscore'; 
import y from './baz';
import A from './byz';

es5

const lib = require('./foo');
const x = require('./bar');
const z = require('./bar');
const F = require('./bar');
const _ = require('underscore');
const y = require('./baz');
const A = require('./byz');

Examples fixed

es6

import _ from 'underscore'; //TBD if * over _ or configurable
import * as lib from './foo';
import A from './byz';
import { x, z , F } from './bar'; //inline dependencies possible caveat TBD
import y from './baz';

es5

const _ = require('underscore');
const A = require('./byz');
const F = require('./bar');
const lib = require('./foo');
const x = require('./bar');
const y = require('./baz');
const z = require('./bar');

Whether the rule is trying to prevent an error or is purely stylistic.

Purely stylistic

Why you believe this rule is generic enough to be included.

The rule would basically collect all dependencies and sort them. This is possible for both es5 and es6 using the same rule.

Whether the rule should be on or off by default.

Off by default

@eslintbot

This comment has been minimized.

Copy link

commented Jul 23, 2015

Thanks for the issue! We get a lot of issues, so this message is automatically posted to each one to help you check that you've included all of the information we need to help you.

Reporting a bug? Please be sure to include:

  1. The version of ESLint you are using (run eslint -v)
  2. The source code that caused the problem
  3. The configuration you're using (for the rule or your entire config file)
  4. The actual ESLint output complete with line numbers

Requesting a new rule? Please be sure to include:

  1. The use case for the rule - what is it trying to prevent or flag?
  2. Whether the rule is trying to prevent an error or is purely stylistic
  3. Why you believe this rule is generic enough to be included

Requesting a feature? Please be sure to include:

  1. The problem you want to solve (don't mention the solution)
  2. Your take on the correct solution to problem

Including this information in your issue helps us to triage it and get you a response as quickly as possible.

Thanks!

@gyandeeps gyandeeps added the triage label Jul 23, 2015

@madeiras madeiras changed the title New Rule: sort-dependencies New Rule suggestion: sort-dependencies Jul 23, 2015

@bgw

This comment has been minimized.

Copy link
Contributor

commented Jul 23, 2015

A common pattern is also for people to group dependencies and then sort within those groups. Eg. third party dependencies followed by a blank line and then local dependencies:

var _ = require('underscore');
var eslint = require('eslint');

var database = require('./database');
var fizzbuzzer = require('../fizzbuzzer');

It'd be nice if this theoretical rule had an option to only check groups of require statement that don't have blank lines between them.

@madeiras

This comment has been minimized.

Copy link
Author

commented Jul 23, 2015

+1, I guess it all boils down to performance

@xjamundx

This comment has been minimized.

Copy link
Contributor

commented Jul 23, 2015

Sounds like this is some mix of sort-vars and no-mixed-requires? Would a mutli-line sort-vars and an additional sort-imports solve be good enough?

@madeiras

This comment has been minimized.

Copy link
Author

commented Jul 24, 2015

My initial idea was kind of a sort-imports ended up by calling it dependencies as I thought it was "more correct"

@nzakas

This comment has been minimized.

Copy link
Member

commented Jul 24, 2015

Seems like sort-vars would already solve the case of require? We can do sort-imports, I just wouldn't mix in dealing with require

@madeiras

This comment has been minimized.

Copy link
Author

commented Jul 25, 2015

That reduces the scope and probably simplifies, makes everyone happy I guess:)

@nzakas

This comment has been minimized.

Copy link
Member

commented Jul 27, 2015

@JoaoHenriquePereira do you want to try creating this?

@madeiras

This comment has been minimized.

Copy link
Author

commented Jul 27, 2015

@nzakas yes I already forked the repo I'm just waiting for that extra time to do it. Probably by the end of this week.

@nzakas

This comment has been minimized.

Copy link
Member

commented Jul 27, 2015

👍

@nzakas nzakas added enhancement rule accepted and removed triage labels Jul 27, 2015

@sindresorhus

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2015

I'm looking for a way to enforce the sorting of imports/requires based on grouping, not alphabetically.

Similar to no-mixed-requires I would like the following sorting enforced:

var fs = require('fs');        // core
var async = require('async');  // module
var foo = require('./');       // main
var bar = require('./foo');    // file
var baz = require(getName());  // computed

Could this potentially be an option in sort-imports rule or would you prefer I open a new rule proposal?

@madeiras

This comment has been minimized.

Copy link
Author

commented Aug 22, 2015

@sindresorhus Hey,

At the moment my current prototype does not consider what you propose, I will probably take a look at it tomorrow and I might check to see whats possible, but I'd like to hear more people's opinions as well.

@necolas

This comment has been minimized.

Copy link

commented Oct 7, 2015

This is how we order imports differently to the "fixed ES6" example:

import { a, b, c } from './abc'
import _ from 'underscore'
import * as lib from './lib'
import A from './foo'
import B, { D } from './bar'
import C from './baz';
@nzakas

This comment has been minimized.

Copy link
Member

commented Oct 7, 2015

@necolas how does that affect this issue?

@nzakas nzakas added help wanted feature and removed enhancement labels Oct 7, 2015

@necolas

This comment has been minimized.

Copy link

commented Oct 8, 2015

Well, we consider our imports to be alpha-sorted (destructured imports go first); but they're not in the same order as the OP's "fixed" example.

@nzakas

This comment has been minimized.

Copy link
Member

commented Oct 8, 2015

@necolas sorry, I'm not being very clear. What I'd like to know is how your pattern is expected to influence this issue? Are you saying we should implement your way of doing this? Are you saying there should be an option? Are you saying you wouldn't be able to use this rule but would like to?

@necolas

This comment has been minimized.

Copy link

commented Oct 9, 2015

I would like to be able to use the rule. If the order were not customisable and differed from what we do now, then I'd make the case for us to adopt whatever the eslint rule enforces.

@renke

This comment has been minimized.

Copy link

commented Oct 30, 2015

I am not sure how relevant it is, but I wrote a node package that sorts ES6 imports (also look here for a better description. The order is customizable (although this mechanism is not really exposed at the moment). Maybe this can be used to implement the import sort rule and a corresponding auto fix.

nzakas added a commit that referenced this issue Jan 11, 2016

Merge pull request #4820 from cschuller/issue-3143
New: 'sort-imports' rule (refs #3143)
@nzakas

This comment has been minimized.

Copy link
Member

commented Jan 11, 2016

Closed in #4820. (Missed having "fixes" in the commit message.)

@wmertens

This comment has been minimized.

Copy link

commented Apr 18, 2016

@cschuller any chance of an auto-fix? Don't feel up to implementing that myself, so just requesting in the hope you do 😉

@renke

This comment has been minimized.

Copy link

commented Apr 18, 2016

@wmertens I hope that one day I find the time to add a style for import-sort that is compatible with the sort-dependencies rule. Better yet, it should read your config and sort accordingly.

@cschuller

This comment has been minimized.

Copy link
Contributor

commented Apr 20, 2016

@wmertens Sorry, the chance is very low, but not zero 😉 Speaking personally, I love ESLint, but I do not like the auto-fix feature. The primary focus of ESLint shall be linting not fixing. Besides my personal point of view, if more people are interested in adding auto-fix to sort-import, I can do it. 😃

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.