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

order accepts regular expressions. Add alphabetize option. #12

Closed
wants to merge 1 commit into from

Conversation

duncanbeevers
Copy link

This PR follows-on to #8

I originally wrote the external-submodules functionality to try and enforce the loosely-followed rules already present in our large webpack-based codebase, but found the rough, pre-defined clustering of rules baked into the plugin to be too restricting to deal with what we were already doing.

To deal with this, I propose this backwards-compatible solution. Like #8 it supports partial orderings of the built-in ordering tiers. Unlike #8, it adds no new explicit tiers. Instead, it allows users to configure their own tiers using regular expressions, and to freely intermingle these regular expressions with the pre-defined tiers.

Additionally, it adds a new alphabetize option for sorting intra-tier modules which helps establish a truly canonical import ordering rather than just rough clustering.

Probably the biggest caveat with this PR is that it significantly relaxes the JSON schema requirements that were previously present, possibly opening an avenue to confusing misconfiguration.

@sindresorhus
Copy link

Can you show a realistic example of the regex ordering? I can't imagine anyone would do [/^lodash\./, /^jquery$/]. I'm having a hard time seeing the use-case.

@jfmengels
Copy link
Owner

I personally like to put lodash/lodash.fp as the first external import, and I wouldn't mind ordering all lodash submodules directly underneath it (though I'll probably stick to whatever XO decides).

Looks promising at first sight :) Will look at it more when I have more time.
Am I right in thinking this does not solve your problem of external submodules?

the biggest caveat with this PR is that it significantly relaxes the JSON schema

Yeah, I've thought about other ways to make it more flexible, but if it's good enough, why not.

@duncanbeevers
Copy link
Author

duncanbeevers commented Apr 15, 2016

@sindresorhus I can't really point out a specific example other than to say that this is an ad-hoc rule in our codebase. We roughly try to order our imports from most-general to most-specific, and in our reasoning, lodash is more general than jQuery.

    "import-order/import-order": [2, { "order": [
        /^yargs$/,
        /^gulp-util$/,
        /\/package.json$/,
        /^\.\/.*\.tpl$/, // Handlebars templates
        /^underscore$/,
        /^jquery$/,
        /^plex\/base\/utils\/log\/log$/, // sort `log` before reporters
        /^plex\/base\/utils\//, // sort base `utils` before `commands`
        /^plex\/base\//,
        /^plex\/desktop\//, // sort desktop dependencies before sub-platforms
        /^plex\/tv\//, // sort TV dependencies before tv sub-platforms
        /^plex/,
        'builtin',
        'external',
        /\/platforms$/,
        /^\.\//, // sibling
        /^\.\.\/[^\.]/, // parent-1
        /^\.\.\/\.\.\/[^\.]/, // parent-2
        /^\.\.\/\.\.\/\.\.\/[^\.]/, // parent-3
        /^\.\.\/\.\.\/\.\.\/\.\.\/[^\.]/ // parent-4
    ], "alphabetize": true }],

@duncanbeevers
Copy link
Author

duncanbeevers commented Apr 15, 2016

@jfmengels This does solve my issue of external submodules, since anything referred-to by a regexp in an otherwise empty order option causes those modules to sort to the top.

The only difficulty I've encountered with this setup is that we generally try to sort static assets (like .css, .png, .svg, etc...) to the bottom of our imports, and negating stuff is obviously a hassle (but not impossible) with regexps.

My goal is to introduce this to our codebase with as few changes as possible to the corpus and just enforce the soft rules by which we already try to abide in order to avoid having to discuss syntax in PRs.

@duncanbeevers
Copy link
Author

@jfmengels @sindresorhus I think introducing an option for an entirely canonical ordering, rather than the current partial-ordering should make auto-fixing simpler down the road.

@jamestalmage
Copy link

jamestalmage commented Apr 15, 2016

Personally, I think this is taking things way overboard. With the given example, I have basically no chance of getting the import order right on my own. yargs comes before builtins but builtins come before other externals? How am I supposed to remember that?

I think the loose partial ordering behavior we have now is ideal. I can quickly zero in on the answers to any number of questions, like: "What external modules does this pull in?" and "What local modules am I testing here?". The rules are simple and straightforward enough for me to keep in my brain, and infer from the surrounding code.

Alphabetizing makes the order a bit more explicit, but isn't super helpful. If we alphabetize according to the variable name, then renaming that variable might mean changing it's position (which feels wrong). If you alphabetize according to package/file name, that will likely create a weird almost-alphabetical ordering of your variable names.

I think trying to enforce a 100% explicit ordering is going to be just too frustrating for people trying to contribute to my projects. If you are new to the rule and have just 4 or 5 badly out of order imports at the top of the file, it's going to take you a number of tries to get the ordering right.

Once ESLint introduces better auto-fixing, I think my attitude might change (at least on adding alphabetize).

@duncanbeevers
Copy link
Author

I agree such a peculiar and draconian ruleset is not appropriate for most projects. The unwieldiness of the example points out the difficulty in trying to use a tool like this to enforce a set of ordering standards that have grown up organically in a mature codebase.

I imagine typical configurations won't have to touch this option at all, and those that do will likely add few explicit rules, relying on the already-good defaults this plugin ships with.

@jamestalmage
Copy link

points out the difficulty in trying to use a tool like this to enforce a set of ordering standards that have grown up organically in a mature codebase

Why stick with it though? Write a codemod that fixes import order to something more logical across your entire codebase than continuing to live with a ruleset you find "peculiar and draconian".

@forivall
Copy link

forivall commented Apr 26, 2016

A much simpler use case is one where I want .json imports to occur last, which would be

['builtin', 'external', 'parent', 'sibling', 'index', /\/.json$/]

since I want to load my config after my actual code, since i'm using require as sugar for config = JSON.parse(fs.readFileSync(path.join(__dirname, '../config.json')))

Now, I haven't tested if this would work with this pr, but ¯_(ツ)_/¯. The other solution would to just add another category like "assets" which would be imports with a specific suffix (like .json, .css, .png, .svg), etc. Or any suffix, and add a eslint rule to warn against ".js" and ".coffee", etc. imports

@jfmengels
Copy link
Owner

Closing this, as I've deprecated this plugin in favor of using eslint-plugin-import. Thanks for the interest nonetheless. If you are still interested in adding this feature, you're free to to create an issue over there or create a new repo with your version of this rule.

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

Successfully merging this pull request may close these issues.

None yet

5 participants