humanmade / coding-standards

Human Made coding standards for modern code
https://engineering.hmn.md/how-we-work/style/
153 stars 19 forks source link

Simplify import order rules #297

Open rmccue opened 2 years ago

rmccue commented 2 years ago

Way back in https://github.com/humanmade/coding-standards/issues/84 and https://github.com/humanmade/coding-standards/pull/219 we added rules for the order of imports.

The way these rules were implemented is kind of complex, and doesn’t match up with the original intention I suggested in the discussion for #84; partially, this is due to the import/order rule being kinda complex to use.

To reiterate my original suggestion: the core intention of ordering rules is to create clarity and have an unambiguous order that’s easily understandable. To do this, we want three groups of imports: things we don’t control (external packages), things we control (internal files), and side-effects imports (like import ‘./foo.css’).

Our current rules create distinctions within these groups that can be tough to understand. They require eg separating import … from ’fs’ from import … from ‘react’, which requires that you know which modules are Node built-ins and which are node_modules. They also separate import x from ‘./foo’ and import x from ‘../foo’ with no real distinction between these, as well as separating imports of index files; it’s unclear what value this provides, and it can be confusing to learn.

They also separate @wordpress/… imports from the others; this was intentional as these packages are provided by the externals configuration and loaded from WordPress’ built-in files rather than npm. Whether we want to keep this is up for discussion; one downside is the blanket rule also covers the edge cases where you do have to load these via npm, so can be confusing.

Proposal

I would like to simplify the rules to create only three groups:

  1. External packages imported by name (e.g. import … from ‘some_name’ or import … from ‘@webpack/foo’)
  2. Internal files imported by path (e.g. import … from ‘./foo’, import … from ‘../foo’, import … from ‘./index’)
  3. Side-effects (e.g. import ‘fix-the-stuff’, import ‘./foo.css’)

As with the current rules, since side-effects are order-dependent, they can be placed anywhere; however, by convention, non-order-dependent imports should be separated.

(As noted above, we may want to keep @wordpress/ separate; I think for consistency with Gutenberg, these imports would want to be first overall?)

Under the current rules, the following is correct:

import path from 'path';

import chalk from 'chalk';
import eslint from 'eslint';

import apiFetch from '@wordpress/api-fetch';

import index from '../../index';
import '../test-lint-config';

import Test from './component-jsx-parentheses';

import './';
import './style.scss';

With this proposal, the following would instead be correct:

import apiFetch from '@wordpress/api-fetch';
import chalk from 'chalk';
import eslint from 'eslint';
import path from 'path';

import index from '../../index';
import Test from './component-jsx-parentheses';

import '../test-lint-config';
import './';
import './style.scss';

(Note: the side-effects could be anywhere if they need to be, but things like CSS can be all in the one group.)

(I would love to make ../ sort after ./ so that “closer” files are sorted first, but import/order doesn’t have any configuration for this.)

rmccue commented 2 years ago

Also: I noticed we never actually documented either the rules or the side-effect convention, so we should ensure those also make their way back into the JS style guide!