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

ESLint rule for enforcing order of require() calls #62

Closed
sindresorhus opened this issue Mar 22, 2016 · 11 comments
Closed

ESLint rule for enforcing order of require() calls #62

sindresorhus opened this issue Mar 22, 2016 · 11 comments

Comments

@sindresorhus
Copy link
Owner

I need an ESLint plugin with a rule for enforcing the following order for require calls:

var fs = require('fs');         // builtins first
var chalk = require('chalk');   // external modules
var foo = require('../../foo'); // relative in another directory
var moo = require('../../');    // index in another directory
var bar = require('./bar');     // relative in the same directory
var main = require('./');       // the index file

If it ends up good, I plan to depend on it in XO.

Anyone interested?

I don't have time to mentor so probably better for someone with previous experience with AST's or strong willingness to learn :)

You can check if something is a builtin with https://github.com/sindresorhus/is-builtin-module.

// @jfmengels @twada @jamestalmage @dustinspecker

@jamestalmage
Copy link

I just want to clear up some ambiguities:


require('./foo');

This can mean ./foo.js or ./foo/index.js. You could solve this with calls to path-exists (the module resolution search order is well defined), but is it worth the complexity it would add to the rule?


You should specify ordering if multiple indexes are required. (index in another directory vs index in the same directory).


Any value in further ordering the require calls within that grouping? Alphabetically? Or (my preference) stair-stepped:

var fs = require('fs');
var events = require('events');
var childProcess = require('child-process');
var chalk = require('chalk');
var isPromise = require('is-promise');
var objectAssign = require('object-assign');

Finally, IMO this rule should work with ES2015 import statements as well.

@sindresorhus
Copy link
Owner Author

This can mean ./foo.js or ./foo/index.js. ... but is it worth the complexity it would add to the rule?

It doesn't really matter and no.

You should specify ordering if multiple indexes are required. (index in another directory vs index in the same directory).

Updated the example.

Any value in further ordering the require calls within that grouping? Alphabetically? Or (my preference) stair-stepped:

Maybe alphabetically on the require string not the variable name?

That way this would sort correctly:

var apple = require('lodash/apple');
var pear = require('lodash/pear');

I don't really see the value in per-group stair-stepping. It creates for some noisy zigzagging. I think I prefer the randomness of alphabetic sorting instead. But happy to be convinced otherwise.

Finally, IMO this rule should work with ES2015 import statements as well.

Ah yes. Definitely.

@jfmengels
Copy link

I'm interested on working on this :)

Any value in further ordering the require calls within that grouping? Alphabetically? Or (my preference) stair-stepped

I think it'd be nice if there was no further ordering by default, but that it could take a parameter allowing it (with built-in alphabetical, and why not stair case).

Any thoughts on ordering the following?

var foo = require('../../foo'); // relative in another directory
var bar = require('./bar');     // relative in the same directory
var bar = require('./bar/baz');     // relative in subdirectory from the same directory

@jamestalmage
Copy link

I don't really see the value in per-group stair-stepping.

There isn't a whole lot, except it does give you visual clues as to group boundaries. I usually just stair-step everything instead of grouping the way you do. Not a big deal to me. Alphabetical is probably better anyways, it would be the easiest to explain to new users.

Maybe alphabetically on the require string not the variable name?

Hmm. Seems like that could cause confusion. I'm starting to think "ordering within ordered groups" is just going to be too much of a burden on contributors.

@jamestalmage
Copy link

We should definitely support the --fix option for this rule.

@sindresorhus
Copy link
Owner Author

Alright. Let's just drop per-group ordering. I agree it would cause a lot of moot overhead for contributors.

@jfmengels
Copy link

var foo = require('../../foo'); // relative in another directory
var moo = require('../../');    // index in another directory

I find these two cases to be hardly different. The latter is an index in a strictly parent directory, while the former is not necessarily an index (but it can be, as it can point to ../../foo/index.js).

@jfmengels
Copy link

✨ Tada! ✨ Let me know what you think if in the issues of the project: https://www.npmjs.com/package/eslint-plugin-import-order.

@sindresorhus
Copy link
Owner Author

@jfmengels Looks awesome! Thanks for taking on this 🎉🦄👍

Would you be interested in doing a PR to XO adding it there? See https://github.com/sindresorhus/xo/blob/master/config/plugins.js

@jfmengels
Copy link

Sure, will do that later :)

@Qix-
Copy link
Collaborator

Qix- commented Mar 27, 2016

Woo!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants