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

RFC: Sort Import Order (Prettier) #997

Open
KevinWu098 opened this issue Aug 13, 2024 · 2 comments
Open

RFC: Sort Import Order (Prettier) #997

KevinWu098 opened this issue Aug 13, 2024 · 2 comments
Assignees
Labels
RFC Request for comment

Comments

@KevinWu098
Copy link
Member

Description

These days, it's a super big DX thing for me to have imports be consistently grouped and ordered for easier parsing when working in (and across) file contexts.

My personal preference is to use https://github.com/IanVS/prettier-plugin-sort-imports, which builds upon https://github.com/trivago/prettier-plugin-sort-imports for a pretty quick plug-and-play experience in managing imports.

Example:

Config:

//@ts-check

/**
 * @type {import('prettier').Config}
 */
const config = {
    plugins: ['@ianvs/prettier-plugin-sort-imports'],
    importOrder: [
        '^(react/(.*)$)|^(react$)',
        '',
        '^(@mui/(.*)$)|^(@material-ui/(.*)$)',
        '',
        '<THIRD_PARTY_MODULES>',
        '',
        '^[./]',
    ],
    importOrderParserPlugins: ['typescript', 'jsx', 'decorators-legacy'],
};

module.exports = config;

Before:

import {
    Box,
    Button,
    Dialog,
    DialogActions,
    DialogContent,
    DialogContentText,
    DialogTitle,
    FormControl,
    FormControlLabel,
    Radio,
    RadioGroup,
    TextField,
    Tooltip,
} from '@material-ui/core';
import InputLabel from '@material-ui/core/InputLabel';
import { PostAdd } from '@material-ui/icons';
import { ChangeEvent, useCallback, useEffect, useState } from 'react';

import TermSelector from '../RightPane/CoursePane/SearchForm/TermSelector';
import RightPaneStore from '../RightPane/RightPaneStore';
import { addCustomEvent, openSnackbar } from '$actions/AppStoreActions';
import analyticsEnum, { logAnalytics } from '$lib/analytics';
import { warnMultipleTerms } from '$lib/helpers';
import AppStore from '$stores/AppStore';
import WebSOC from '$lib/websoc';
import { CourseInfo } from '$lib/course_data.types';
import { addCourse } from '$actions/AppStoreActions';
import { ZotCourseResponse, queryZotCourse } from '$lib/zotcourse';
import { useThemeStore } from '$stores/SettingsStore';

After:

import { ChangeEvent, useCallback, useEffect, useState } from 'react';

import {
    Box,
    Button,
    Dialog,
    DialogActions,
    DialogContent,
    DialogContentText,
    DialogTitle,
    FormControl,
    FormControlLabel,
    Radio,
    RadioGroup,
    TextField,
    Tooltip,
} from '@material-ui/core';
import InputLabel from '@material-ui/core/InputLabel';
import { PostAdd } from '@material-ui/icons';

import { addCourse, addCustomEvent, openSnackbar } from '$actions/AppStoreActions';
import analyticsEnum, { logAnalytics } from '$lib/analytics';
import { CourseInfo } from '$lib/course_data.types';
import { warnMultipleTerms } from '$lib/helpers';
import WebSOC from '$lib/websoc';
import { queryZotCourse, ZotCourseResponse } from '$lib/zotcourse';
import AppStore from '$stores/AppStore';
import { useThemeStore } from '$stores/SettingsStore';

import TermSelector from '../RightPane/CoursePane/SearchForm/TermSelector';
import RightPaneStore from '../RightPane/RightPaneStore';

This is a pretty basic example (which we can further configure for our directory structure), but I think this is a solid win for a low effort addition!

@KevinWu098 KevinWu098 self-assigned this Aug 13, 2024
@KevinWu098 KevinWu098 added the RFC Request for comment label Aug 13, 2024
@ap0nia
Copy link
Collaborator

ap0nia commented Aug 14, 2024

Oh actually, we do have sorting; but it's done via an ESLint plugin -- eslint-plugin-import.

An alternative plugin is eslint-plugin-simple-import.

We've configured it to throw linting errors if the imports are out of order:

❯ pnpm eslint src/components/Header/Import.tsx

/home/aponia/Projects/icssc/antalmanac/apps/antalmanac/src/components/Header/Import.tsx
  21:1   error    There should be at least one empty line between import groups                                                    import/order
  22:46  warning  '/home/aponia/Projects/icssc/antalmanac/apps/antalmanac/src/actions/AppStoreActions.ts' imported multiple times  import/no-duplicates
  26:1   error    `$lib/websoc` import should occur before import of `$stores/AppStore`                                            import/order
  26:8   warning  Using exported name 'WebSOC' as identifier for default export                                                    import/no-named-as-default
  27:1   error    `$lib/course_data.types` import should occur before import of `$lib/helpers`                                     import/order
  28:1   error    `$actions/AppStoreActions` import should occur before import of `$lib/analytics`                                 import/order
  28:27  warning  '/home/aponia/Projects/icssc/antalmanac/apps/antalmanac/src/actions/AppStoreActions.ts' imported multiple times  import/no-duplicates
  29:1   error    `$lib/zotcourse` import should occur before import of `$stores/AppStore`                                         import/order

✖ 8 problems (5 errors, 3 warnings)
  5 errors and 1 warning potentially fixable with the `--fix` option.

If you run pnpm lint --fix, then it will re-format the file to follow the configured rules.

Aside from being biased, I think it sorta makes more sense for the linter to make semantic decisions about the code since formatters are generally pretty dumb.


I do think the default order from the prettier plugin seems to make a lot more sense to me than the eslint plugin's for some reason lol. (eslint-plugin thinks that $stores should go before ../relative/path 🤷 ). This can be configured via the order setting, but I'm not sure if it's worth the effort to make a custom config 🤔

eslint-plugin-import suggested import order

import {
    Box,
    Button,
    Dialog,
    DialogActions,
    DialogContent,
    DialogContentText,
    DialogTitle,
    FormControl,
    FormControlLabel,
    Radio,
    RadioGroup,
    TextField,
    Tooltip,
} from '@material-ui/core';
import InputLabel from '@material-ui/core/InputLabel';
import { PostAdd } from '@material-ui/icons';
import { ChangeEvent, useCallback, useEffect, useState } from 'react';

import TermSelector from '../RightPane/CoursePane/SearchForm/TermSelector';
import RightPaneStore from '../RightPane/RightPaneStore';

import { addCustomEvent, openSnackbar , addCourse } from '$actions/AppStoreActions';
import analyticsEnum, { logAnalytics } from '$lib/analytics';
import { CourseInfo } from '$lib/course_data.types';
import { warnMultipleTerms } from '$lib/helpers';
import WebSOC from '$lib/websoc';
import { ZotCourseResponse, queryZotCourse } from '$lib/zotcourse';
import AppStore from '$stores/AppStore';
import { useThemeStore } from '$stores/SettingsStore';

@KevinWu098
Copy link
Member Author

Ah that's right -- it's been so long since I've worked on the codebase that I forgot about the linter. IIRC it doesn't format on save, but if we already have it, this DX thing might be better accomplished by fussing with a custom config (you have to create a custom order for Prettier anyways)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RFC Request for comment
Projects
Status: Backlog 🥱
Development

No branches or pull requests

2 participants