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.

dvc.org/pages/doc.js

Lines 3 to 24 in 9c8cc54

	import React, { Component } from 'react'
	// nextjs
	import { HeadInjector } from '../src/Documentation/HeadInjector'
	// components
	import SidebarMenu from '../src/Documentation/SidebarMenu/SidebarMenu'
	import Markdown from '../src/Documentation/Markdown/Markdown'
	import RightPanel from '../src/Documentation/RightPanel/RightPanel'
	import Page from '../src/Page'
	import SearchForm from '../src/SearchForm'
	import Page404 from '../src/Page404'
	import Loader from '../src/Loader/Loader'
	import Hamburger from '../src/Hamburger'
	// utils
	import fetch from 'isomorphic-fetch'
	import kebabCase from 'lodash.kebabcase'
	// styles
	import styled from 'styled-components'
	import { media } from '../src/styles'
	// sidebar data and helpers
	import sidebar, { getItemByPath } from '../src/Documentation/SidebarMenu/helper'
	// constants
	import { HEADER } from '../consts'

or

dvc.org/src/Documentation/SidebarMenu/SidebarMenu.js

Lines 1 to 14 in 9c8cc54

	import React from 'react'
	import PerfectScrollbar from 'perfect-scrollbar'
	import scrollIntoView from 'dom-scroll-into-view'
	import PropTypes from 'prop-types'
	// components
	import DownloadButton from '../../DownloadButton'
	// utils
	import includes from 'lodash.includes'
	// styles
	import styled from 'styled-components'
	import { media, OnlyDesktop } from '../../styles'
	// sidebar helpers
	import { getParentsListFromPath } from './helper'

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.

[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]

p.s. actually I checked and fortunately, the commits that changed this in that big PR are consecutive: 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 7ea2edb (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.