Add sort-import ESLint rule

[benoitblanc]

@manisandro what do you think about this configuration ?

allowSeparatedGroups could allow us to import style at the end of the imports.

It sorts alphabetically Capital letters, then lowercase letters.

The member syntax sort order is

• none - import module without exported bindings.
• single - import single member.
• multiple - import multiple members.
• all - import all members provided by exported bindings.

It does not take into account relative or absolute imports.

For example, imports of Bookmark plugin with this rule:

import ConfigUtils from '../utils/ConfigUtils';
import Icon from '../components/Icon';
import LocaleUtils from '../utils/LocaleUtils';
import PropTypes from 'prop-types';
import React from 'react';
import SideBar from '../components/SideBar';
import Spinner from '../components/Spinner';
import classnames from 'classnames';
import {connect} from 'react-redux';
import isEmpty from 'lodash.isempty';
import {createBookmark, getUserBookmarks, removeBookmark, updateBookmark} from '../utils/PermaLinkUtils';

import './style/Bookmark.css';

Thanks

[manisandro]

allowSeparatedGroups should also allow us to keep the absolute imports on top and the relative imports separately, right?

[benoitblanc]

Yes but it needs to be separated by a blank line or a comment line.

For example:

import ConfigUtils from '../utils/ConfigUtils';
import Icon from '../components/Icon';
import LocaleUtils from '../utils/LocaleUtils';
import PropTypes from 'prop-types';
import React from 'react';
import SideBar from '../components/SideBar';
import Spinner from '../components/Spinner';
import classnames from 'classnames';
import isEmpty from 'lodash.isempty';

import {connect} from 'react-redux';
import {createBookmark, getUserBookmarks, removeBookmark, updateBookmark} from '../utils/PermaLinkUtils';

import './style/Bookmark.css';

or something like that (with absolute/relative and single/mulitple)

import PropTypes from 'prop-types';
import React from 'react';
import classnames from 'classnames';
import isEmpty from 'lodash.isempty';

import {connect} from 'react-redux';

import ConfigUtils from '../utils/ConfigUtils';
import Icon from '../components/Icon';
import LocaleUtils from '../utils/LocaleUtils';
import SideBar from '../components/SideBar';
import Spinner from '../components/Spinner';

import {createBookmark, getUserBookmarks, removeBookmark, updateBookmark} from '../utils/PermaLinkUtils';

import './style/Bookmark.css';

[manisandro]

This should also work right?

import PropTypes from 'prop-types';
import React from 'react';
import classnames from 'classnames';
import isEmpty from 'lodash.isempty';
import {connect} from 'react-redux';

import ConfigUtils from '../utils/ConfigUtils';
import Icon from '../components/Icon';
import LocaleUtils from '../utils/LocaleUtils';
import SideBar from '../components/SideBar';
import Spinner from '../components/Spinner';
import {createBookmark, getUserBookmarks, removeBookmark, updateBookmark} from '../utils/PermaLinkUtils';

import './style/Bookmark.css';

[benoitblanc]

This should also work right?

import PropTypes from 'prop-types';
import React from 'react';
import classnames from 'classnames';
import isEmpty from 'lodash.isempty';
import {connect} from 'react-redux';

import ConfigUtils from '../utils/ConfigUtils';
import Icon from '../components/Icon';
import LocaleUtils from '../utils/LocaleUtils';
import SideBar from '../components/SideBar';
import Spinner from '../components/Spinner';
import {createBookmark, getUserBookmarks, removeBookmark, updateBookmark} from '../utils/PermaLinkUtils';

import './style/Bookmark.css';

No because named imports are also sorted with default imports (so {connect} should be before isEmpty).

We would have to create one block for each of these import types:

// absolute single default imports
import PropTypes from 'prop-types';
import React from 'react';
import classnames from 'classnames';
import isEmpty from 'lodash.isempty';
// absolute single named imports
import {connect} from 'react-redux';
// absolute multiple named imports

// relative single default imports
import ConfigUtils from '../utils/ConfigUtils';
import Icon from '../components/Icon';
import LocaleUtils from '../utils/LocaleUtils';
import SideBar from '../components/SideBar';
import Spinner from '../components/Spinner';
// relative single named imports
// relative multiple named imports
import {createBookmark, getUserBookmarks, removeBookmark, updateBookmark} from '../utils/PermaLinkUtils';

// styles
import './style/Bookmark.css';

[manisandro]

Right. So the convention I've been using up to now was

• Globals first, order pretty random
• Relatives next, sorted (more or less) by the imported path, and then by the imported member(s)
• CSS last

I guess there is no rule to achieve this anyway, so boh dunno, no strong feelings except perhaps wanting to minimize the about of work to clean up all existing source files... But ultimately happy with whatever if we decide to add a rule for this.

[benoitblanc]

I think the last one in #317 (comment) could be a good one, I can do the changes in existing src files (in this same PR ?). We could also keep the comments for contributors to separate blocks.

What do you think ?

[manisandro]

Yes ok for me!

[benoitblanc]

As it it really laborious to change existing source code to fix errors, I think we could introduce an ESLint plugin to be able to automatically fix errors. For instance, I have found this one: https://github.com/azat-io/eslint-plugin-perfectionist where we can just introduce sort-imports rule for now https://eslint-plugin-perfectionist.azat.io/rules/sort-imports, and we could add some other rules later.

[manisandro]

Makes sense - I guess this could then be configured as a pre-commit hook

[github-advanced-security]

ESLint found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

[benoitblanc]

I have added the plugin with a rule and run ESLint with --fix option on all the code in qwc2 to sort imports (with small fixes on indents or missing semi-column).

I think you have to run ESLint on .eslint.js to fix indentations directly on master branch because if I do it in this PR, I have conflicts between my .eslintrc.js and the one that is running in CI.

[manisandro]

Thanks - so this is good to merge now?

[benoitblanc]

Yes :-)

[manisandro]

Top, thanks!

[manisandro]

Hmm do you still get the eslint warnings in the code editor (i.e. vscodium)?

[benoitblanc]

Hmm do you still get the eslint warnings in the code editor (i.e. vscodium)?

Yes, I had to restart ESLint server

[manisandro]

Right, working now, thanks!