Dvc.org: app: sort es6 imports

[jorgeorpinel]

Currently there's not much order in js file imports. Some files group them with comments or spaces in between e.g.

https://github.com/iterative/dvc.org/blob/9c8cc540cfbd004db9ac3747bd56b6ddc328ed24/pages/doc.js#L3-L24

or

https://github.com/iterative/dvc.org/blob/9c8cc540cfbd004db9ac3747bd56b6ddc328ed24/src/Documentation/SidebarMenu/SidebarMenu.js#L1-L14

I suggest we remove all these comments, which don't really add any value, and apply this ESLint rule: https://eslint.org/docs/rules/sort-imports

[shcheklein]

@jorgeorpinel 👍 let's try.

[jorgeorpinel]

Alright, I added the rule and ran eslint --fix pages src in 327d5837 but this only fixes some of the problems found by it. (Prettier/Restyled can't fix this either.)

Most issues have to be solved manually and are still outstanding, so I left the rule with "warn" level (not "error"), so it doesn't make the CI checks fail. This means unfortunately no one will ever notice these warnings (they're not even displayed by lint-staged when it runs ESLint, since warnings don't cause it to exit with a non-zero code and the output is hidden).

Should it be mandatory instead @shcheklein? This way it's really only useful if you manually run yarn lint-check and fix manually.

[jorgeorpinel]

Also, VSCode has a sort-imports extension that fixes the problems eslint --fix can't. We could add it as a recommendation in the docs contrib guide instead, or on top of making the ESLint rule mandatory.

Note that it also applies this and this styles.

[iAdramelk]

@jorgeorpinel @shcheklein hi guys, missed the discussion. I agree that the old sorting system was not ideal, but current version in master is even more chaotic and hard to work with.

For example docs.js:

import React, { useCallback, useEffect, useState } from 'react'
import { getItemByPath, structure } from '../src/utils/sidebar'

import Error from 'next/error'
import Hamburger from '../src/Hamburger'
import { HeadInjector } from '../src/Documentation/HeadInjector'
import Markdown from '../src/Documentation/Markdown/Markdown'
import Page from '../src/Page'
import PropTypes from 'prop-types'
import RightPanel from '../src/Documentation/RightPanel/RightPanel'
import Router from 'next/router'
import SearchForm from '../src/SearchForm'
import SidebarMenu from '../src/Documentation/SidebarMenu/SidebarMenu'
import fetch from 'isomorphic-fetch'
import kebabCase from 'lodash.kebabcase'
import { media } from '../src/styles'
import styled from 'styled-components'

Libraries are sorted in the middle of our local files. Utils are sorted in between components, styles are in some random place. Also sorting algorithm is pretty strange: Right now it is using only "filename" for sorting, and ignores everything else. As a result ../src/Documentation/RightPanel/RightPanel and ../src/Documentation/SidebarMenu/SidebarMenu are separated by next/router and ../src/SearchForm. We really should either change sorting algorithm or revert this change.

I think at least we want to have all npm libraries grouped before our own files and if we want to sort our own files we want to use the full path and not only filename.

[iAdramelk]

P. S. Also I don't understand how it sorted isomorphic-fetch and lodash.kebabcase to their current position at all.

[jorgeorpinel]

current version in master is even more chaotic... For example docs.js... Libraries are sorted in the middle of our local files... Utils are sorted in between components, styles are in some random place... (etc.)

May not be obvious what the ordering is based on but we're just using default https://eslint.org/docs/rules/sort-imports now.

Right now it is using only "filename" for sorting, and ignores everything else

Not exactly. Here's the rule's description:

This rule checks all import declarations and verifies that all imports are first sorted by the used member syntax and then alphabetically by the first member or alias name...
There are four different styles and the default member syntax sort order is:

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

There's more linked documentation explaining the criteria, it also has to do with which imports have side-effects from what I remember, and other considerations.

and hard to work with

Why harder though? You just add the import and ESLint keeps it sorted for you.

We really should either change sorting algorithm or revert this change.

Maybe just changing the rules's config will yield a nicer result? Open to improvements and suggestions of course. But the change is part of a huge PR (#882) unfortunately, so not feasible to just revert at this point.

[shcheklein]

@jorgeorpinel so, not sure how is it different. It looks like it's indeed an alphabetical sort - which looks like random to be honest.

We should have disabled ignoreDeclarationSort most likely.

[shcheklein]

I've reopened it to keep discussion going.

[jorgeorpinel]

It's different in that there is a criteria now (a default best practice), and that it gets enforced automatically.

We can definitely try to improve the configuration so the automatic sorting criteria is more obvious and the result looks nicer.

[shcheklein]

@jorgeorpinel having a single standard is great, but not every criteria can be considered reasonable. I agree with @iAdramelk that sorting by name == random and it makes it hard to work with (read and understand code), hard to maintain it manually, etc.

[jorgeorpinel]

We should have disabled ignoreDeclarationSort most likely.

I think you mean set it to true (It's false by default). Yes, that should prevent the alphabetical sorting.

having a single standard is great, but not every criteria can be considered reasonable... sorting by name == random and it makes it hard to work with (read and understand code)

So the main complaint is about alphabetical sort, right? { "memberSyntaxSortOrder": ["none", "all", "multiple", "single"] } seems fine to you? (See https://eslint.org/docs/rules/sort-imports#membersyntaxsortorder)

In any case, since reverting automatically isn't really an option at this point, we'd have to manually re-sort all the imports. I'm happy to work on it but I need to know what the criteria should be because you guys have clearly stated what's wrong with the default ESLint one, (and maybe its obvious to you) but I don't know what the criteria should be.

[jorgeorpinel]

p.s. actually I checked and fortunately, the commits that changed this in that big PR are consecutive: https://github.com/iterative/dvc.org/compare/43303c0...b633778 so it may be possible to revert with git revert? Not sure. But it would bring back al the unnecessary comments between import statements.

[shcheklein]

I think you mean set it to true (It's false by default)

Yes

but I don't know what the criteria should be.

I would expect them to be split into block - third party libraries as a separate block and always comes first or last (?), internal imports spilt into components and everything else at least (utils, constants). Not sure if we need to split anything further. @iAdramelk should have a better idea.

Multiple members from the same component - probably one import.

[shcheklein]

But it would bring back al the unnecessary comments between import statements.

it should be easy to just remove those comments

[iAdramelk]

@jorgeorpinel @shcheklein

About my problems. I will add here an example of how I usually sort dependencies in my projects (it's slightly different from dvc.org) and describe why I prefer it.

Just resorting example above.

Current:

import React, { useCallback, useEffect, useState } from 'react'
import { getItemByPath, structure } from '../src/utils/sidebar'

import Error from 'next/error'
import Hamburger from '../src/Hamburger'
import { HeadInjector } from '../src/Documentation/HeadInjector'
import Markdown from '../src/Documentation/Markdown/Markdown'
import Page from '../src/Page'
import PropTypes from 'prop-types'
import RightPanel from '../src/Documentation/RightPanel/RightPanel'
import Router from 'next/router'
import SearchForm from '../src/SearchForm'
import SidebarMenu from '../src/Documentation/SidebarMenu/SidebarMenu'
import fetch from 'isomorphic-fetch'
import kebabCase from 'lodash.kebabcase'
import { media } from '../src/styles'
import styled from 'styled-components'

Preferred by me:

import fetch from 'isomorphic-fetch'
import kebabCase from 'lodash.kebabcase'
import Error from 'next/error'
import Router from 'next/router'
import PropTypes from 'prop-types'
import React, { useCallback, useEffect, useState } from 'react'
import styled from 'styled-components'

import { HeadInjector } from '../src/Documentation/HeadInjector'
import Markdown from '../src/Documentation/Markdown/Markdown'
import RightPanel from '../src/Documentation/RightPanel/RightPanel'
import SidebarMenu from '../src/Documentation/SidebarMenu/SidebarMenu'
import Hamburger from '../src/Hamburger'
import Page from '../src/Page'
import SearchForm from '../src/SearchForm'

import { getItemByPath, structure } from '../src/utils/sidebar'

import { media } from '../src/styles'

There are a few separate groups. Components inside the group are sorted by full import path and not the file name.

When I open such file I can understand a few things from the start:

1. By looking at the global dependencies, I can see:
   
   
   
   • That this is a React component with styles and with props.
   
   
   • The number of external dependencies (if there is a lot of them it is a good flag for refactoring, for example, I'd like to remove lodash in the future to reduce our bundle size).
   
   
   • That it uses some external Next stuff that I'm refactoring in other PR right now.
   
   
2. By looking at the component list, I can understand what this component consists of.
3. A lot of imported components in the folder /Documents/ tells me that there is a lot of intersection between this file and that folder and that this is probably a right place for the refactor (probably make one component at the root of /Document/ folder and import it once it instead of importing them all one by one)
4. Looking for the block with stuff like ./style.js or ./image.png (not in this example) I can understand which parts this component has.

Then I edit this file I:

1. Easily understand if the library or component is already imported, or I need to add it.
2. Know where to find import that I need to remove. (Ok, the linter will show this one, but still)
3. Know there to add new import (I tried to resort file in a new way manually with my branch and gave up after five tries or so).

With a new structure I can't do any of this stuff. Also, this is my first time seeing it in the real project, and none of the popular eslint presets (like airbnb or standard) are using it, so I'm not sure if we can call it industry standard or best practice.

[iAdramelk]

@jorgeorpinel what have we decided on this?

[shcheklein]

I vote for disabling that auto sort asap, reverting if possible some changes (does it make sense, will you be able to merge @iAdramelk ?), and keep rule in mind that Alex described while preserving reasonable sorting manually. As second-order task - check if there are better tools that can understand semantics and maintain a reasonable order.

[jorgeorpinel]

Sorry, haven't had time to get onto this yet but my plan is to revert indeed, and then play with the rule's settings to hopefully come up with an OK standard based on the desired criteria you guys have expressed. If it's getting in your way maybe change the rule to "warning" value in your branch for now @iAdramelk

[iAdramelk]

@jorgeorpinel my current problems is that I want to merge master into community page and it requires solving conflicts with this changes, and if we later revert this commits it will break code again, also I'm working on the new branch now, than will require moving all components to /src/components/ subfolder, and if I do it before we revert this, it will probably create conflict with moved files that we will need to solve manually.

If you don't have time right now, do you mind that I revert them myself in master?

[jorgeorpinel]

Yeah no worries, thanks for reverting. OK sounds like ESLint sort-imports is not going to let us do any of the things you guys have described. It's a completely different logic (by import syntax type and then alphabetically). So let's forget about that rule... (now removed from master)

---

To summarize, sounds like we would prefer to have groups:

• Clearly differentiate between package ("external") imports and "internal" imports.
• Furthermore as this is a React project, we could have a separate group for React components.
  This much I can understand and support and will start implementing.

Questions:

@iAdramelk suggests to sort alphabetically but not based on the module name, rather on the import path. Why is this better Alex? Seems equally arbitrary. Why not forget about alphabetical order altogether and put the more important stuff or the things used first in the code (like React and Next) in the top of the group?

...internal imports spilt into components and everything else at least (utils, constants).

Sorry @shcheklein, didn't get that. What do you mean "spilt into components" and "everything else at least"?

Preferred by me..

I see 4 groups in your example Alex. The last 2 groups (which only have 1 statement each) don't look like obviously different from the 2nd group with all the components.

• '../src/styles' just contains components, why isn't it in the 2nd group?
• Is '../src/utils/sidebar' in it's own group because its not a React component, because it's executed at load time / has side-effects, or for some other reason?

Know there to add new import (I tried to resort file in a new way manually with my branch and gave up after five tries or so).

Well, with an automatic tool you don't have to worry about this. You can put it anywhere and it gets moved to the right place 😉 but I haven't found any other tool. Will research some more.

[jorgeorpinel]

haven't found any other tool. Will research some more.

I found renke/import-sort import-sort-cli and a bunch other similar ones on GH but they're all based onthe ESLint rule. So seems like we would have to do all this manually 🙁

[shcheklein]

@jorgeorpinel close this for now then?

[jorgeorpinel]

To summarize, sounds like we would prefer to have groups... * Clearly differentiate between package ("external") imports and "internal" imports... * have a separate group for React components...

I implemented this for the commonjs style files (server.js, sidebar.js) and for pages/doc.js. For secondary sort order I used first-used instead of some alphabetical order. See 7ea2edbf (part of #915, I'll open a conversation there.)

TBH I think manual sorting it's to "labor-intensive". I don't want to go sort the other 36 files with import nor do I think people will remember to update the sort order when something changes in each one. Just given that I would still prefer the ESLint rule. But anyway, should we just close this ticket and leave things as they are now?

[jorgeorpinel]

close this for now then?

Just noticed that @shcheklein. Yes.