Skip to content

Commit

Permalink
[Fleet] Add lint rule/fix for import order groups (#93300) (#93490)
Browse files Browse the repository at this point in the history
## Problem
Blocks of 10-15 `import`s are common in the plugin and there a few places which have ~50 lines of `import`s. It makes it more difficult to understand the where/why of what's being imported.

We've had instances while files import from the same module in different lines. i.e.

```ts
import { a } from './file';
... 5-10 lines later
import { b } from './file';
```

## Proposed solution
Add a lint rule to enforce a convention on the module `import` order. This can help in the same way Prettier & ESLint help to format type signatures or other code. It makes it easier to understand or notice any changes in the code. It's also able to be fixed automatically (`node scripts/eslint.js --fix` or any existing "format on save" in an editor).

## This PR
replaces #92980 (based on #92980 (review))

### Lint rule
f9be98d Add eslint rule to enforce/autofix import group order. Use the same rule as a few other plugins. Groups `import` statements by type as shown in the [lint rule docs](https://github.com/benmosher/eslint-plugin-import/blob/master/docs/rules/order.md#importorder-enforce-a-convention-in-module-import-order
). The order is:

  1. node "builtin" modules
  2. "external" modules
  3. "internal" modules
  4. modules from a "parent" directory
  5. "sibling" modules from the same or a sibling's directory, "index" of the current directory, everything else

e.g.

```typescript
import fs from 'fs';
import path from 'path';

import _ from 'lodash';
import chalk from 'chalk';

import foo from 'src/foo';

import foo from '../foo';
import qux from '../../foo/qux';

import bar from './bar';
import baz from './bar/baz';
import main from './';
```
The [lint rule](https://github.com/benmosher/eslint-plugin-import/blob/master/docs/rules/order.md#importorder-enforce-a-convention-in-module-import-order) is relatively light handed. It only ensures  the `imports` are groups together in the given order. It doesn't alphabetize or otherwise sort the order of the files.


e.g. imports aren't rewritten to be in alphabetical order. This is fine

```ts
import from './c';
import from './a';
import from './b';
```

The [docs show other options](https://github.com/benmosher/eslint-plugin-import/blob/master/docs/rules/order.md#options) and https://github.com/jfsiii/kibana/blob/2831f02bc7d7d4d6792e980b7733ddcdfc340cea/.eslintrc.js#L1138-L1168 uses many of them

### Newlines option
The newlines settings means a change from something like

```typescript
import fs from 'fs';
import path from 'path';
import _ from 'lodash';
import chalk from 'chalk';
import foo from 'src/foo';
import foo from '../foo';
import qux from '../../foo/qux';
import bar from './bar';
import baz from './bar/baz';
import main from './';
```

to 

```typescript
import fs from 'fs';
import path from 'path';

import _ from 'lodash';
import chalk from 'chalk';

import foo from 'src/foo';

import foo from '../foo';
import qux from '../../foo/qux';

import bar from './bar';
import baz from './bar/baz';
import main from './';
```



Added it as a separate commit 2831f02 in case we want to avoid it, but I believe it's an improvement overall. Especially on the files with 25+ lines of imports. Even the "worst case" of something like this isn't bad (IMO). Especially since it's an automatic reformat like anything else in prettier


```typescript
import fs from 'fs';

import _ from 'lodash';

import foo from '../foo';

import main from './';
```

Co-authored-by: John Schulz <john.schulz@elastic.co>
  • Loading branch information
kibanamachine and John Schulz authored Mar 3, 2021
1 parent d740ddf commit 97ccf1e
Show file tree
Hide file tree
Showing 298 changed files with 664 additions and 182 deletions.
16 changes: 16 additions & 0 deletions .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -793,6 +793,22 @@ module.exports = {
},
},

/**
* Fleet overrides
*/
{
files: ['x-pack/plugins/fleet/**/*.{js,mjs,ts,tsx}'],
rules: {
'import/order': [
'warn',
{
groups: ['builtin', 'external', 'internal', 'parent'],
'newlines-between': 'always-and-inside-groups',
},
],
},
},

/**
* Security Solution overrides
*/
Expand Down
3 changes: 2 additions & 1 deletion x-pack/plugins/fleet/common/constants/agent_policy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,10 @@
* 2.0.
*/

import { defaultPackages } from './epm';
import type { AgentPolicy } from '../types';

import { defaultPackages } from './epm';

export const AGENT_POLICY_SAVED_OBJECT_TYPE = 'ingest-agent-policies';
export const AGENT_POLICY_INDEX = '.fleet-policies';
export const agentPolicyStatuses = {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
*/

import { safeDump } from 'js-yaml';

import type { FullAgentPolicy } from '../types';

const POLICY_KEYS_ORDER = [
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,10 @@
* 2.0.
*/

import { isAgentUpgradeable } from './is_agent_upgradeable';
import type { Agent } from '../types/models/agent';

import { isAgentUpgradeable } from './is_agent_upgradeable';

const getAgent = ({
version,
upgradeable = false,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

import semverCoerce from 'semver/functions/coerce';
import semverLt from 'semver/functions/lt';

import type { Agent } from '../types';

export function isAgentUpgradeable(agent: Agent, kibanaVersion: string) {
Expand Down
1 change: 1 addition & 0 deletions x-pack/plugins/fleet/common/services/license.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
*/

import { Observable, Subscription } from 'rxjs';

import type { ILicense } from '../../../licensing/common/types';

// Generic license service class that works with the license observable
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
*/

import type { PackagePolicy, PackagePolicyInput } from '../types';

import { storedPackagePoliciesToAgentInputs } from './package_policies_to_agent_inputs';

describe('Fleet - storedPackagePoliciesToAgentInputs', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
*/

import type { PackageInfo } from '../types';

import { packageToPackagePolicy, packageToPackagePolicyInputs } from './package_to_package_policy';

describe('Fleet - packageToPackagePolicy', () => {
Expand Down
3 changes: 2 additions & 1 deletion x-pack/plugins/fleet/common/types/models/agent.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,10 @@
* 2.0.
*/

import type { FullAgentPolicy } from './agent_policy';
import { AGENT_TYPE_EPHEMERAL, AGENT_TYPE_PERMANENT, AGENT_TYPE_TEMPORARY } from '../../constants';

import type { FullAgentPolicy } from './agent_policy';

export type AgentType =
| typeof AGENT_TYPE_EPHEMERAL
| typeof AGENT_TYPE_PERMANENT
Expand Down
1 change: 1 addition & 0 deletions x-pack/plugins/fleet/common/types/models/agent_policy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

import { agentPolicyStatuses } from '../../constants';
import type { DataType, ValueOf } from '../../types';

import type { PackagePolicy, PackagePolicyPackage } from './package_policy';
import type { Output } from './output';

Expand Down
2 changes: 2 additions & 0 deletions x-pack/plugins/fleet/common/types/models/epm.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
// Follow pattern from https://github.com/elastic/kibana/pull/52447
// TODO: Update when https://github.com/elastic/kibana/issues/53021 is closed
import type { SavedObject, SavedObjectAttributes, SavedObjectReference } from 'src/core/public';

import {
ASSETS_SAVED_OBJECT_TYPE,
agentAssetTypes,
Expand All @@ -17,6 +18,7 @@ import {
requiredPackages,
} from '../../constants';
import type { ValueOf } from '../../types';

import type { PackageSpecManifest, PackageSpecScreenshot } from './package_spec';

export type InstallationStatus = typeof installationStatuses;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
*/

import type { AgentPolicy, NewAgentPolicy, FullAgentPolicy } from '../models';

import type { ListWithKuery } from './common';

export interface GetAgentPoliciesRequest {
Expand Down
8 changes: 5 additions & 3 deletions x-pack/plugins/fleet/public/applications/fleet/app.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,11 @@ import { FormattedMessage } from '@kbn/i18n/react';
import { i18n } from '@kbn/i18n';
import styled from 'styled-components';
import useObservable from 'react-use/lib/useObservable';

import { FleetConfigType, FleetStartServices } from '../../plugin';
import { KibanaContextProvider } from '../../../../../../src/plugins/kibana_react/public';
import { EuiThemeProvider } from '../../../../../../src/plugins/kibana_react/common';

import {
ConfigContext,
FleetStatusProvider,
Expand All @@ -34,10 +39,7 @@ import { DataStreamApp } from './sections/data_stream';
import { FleetApp } from './sections/agents';
import { IngestManagerOverview } from './sections/overview';
import { ProtectedRoute } from './index';
import { FleetConfigType, FleetStartServices } from '../../plugin';
import { UIExtensionsStorage } from './types';
import { KibanaContextProvider } from '../../../../../../src/plugins/kibana_react/public';
import { EuiThemeProvider } from '../../../../../../src/plugins/kibana_react/common';
import { UIExtensionsContext } from './hooks/use_ui_extension';

const ErrorLayout = ({ children }: { children: JSX.Element }) => (
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import React, { useState } from 'react';
import styled from 'styled-components';
import { FormattedMessage } from '@kbn/i18n/react';
import { EuiText, EuiLink } from '@elastic/eui';

import { AlphaFlyout } from './alpha_flyout';

const Message = styled(EuiText).attrs((props) => ({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import React from 'react';
import styled from 'styled-components';
import { EuiText, EuiSpacer, EuiLink, EuiTitle, EuiCodeBlock } from '@elastic/eui';
import { FormattedMessage } from '@kbn/i18n/react';

import { EnrollmentAPIKey } from '../../../types';

interface Props {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

import React, { memo, ReactNode, Suspense } from 'react';
import { EuiErrorBoundary } from '@elastic/eui';

import { Loading } from './loading';

export const ExtensionWrapper = memo<{ children: ReactNode }>(({ children }) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,10 @@ import React, { memo, useState, useEffect } from 'react';
import { FormattedMessage } from '@kbn/i18n/react';
import { EuiButtonEmpty } from '@elastic/eui';
import type { TutorialDirectoryHeaderLinkComponent } from 'src/plugins/home/public';

import { RedirectAppLinks } from '../../../../../../../../src/plugins/kibana_react/public';
import { useLink, useCapabilities, useStartServices } from '../../hooks';

import { tutorialDirectoryNoticeState$ } from './tutorial_directory_notice';

const TutorialDirectoryHeaderLink: TutorialDirectoryHeaderLinkComponent = memo(() => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import {
EuiSpacer,
} from '@elastic/eui';
import type { TutorialDirectoryNoticeComponent } from 'src/plugins/home/public';

import { RedirectAppLinks } from '../../../../../../../../src/plugins/kibana_react/public';
import {
sendPutSettings,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import React, { memo } from 'react';
import { FormattedMessage } from '@kbn/i18n/react';
import { EuiText, EuiLink, EuiSpacer } from '@elastic/eui';
import { TutorialModuleNoticeComponent } from 'src/plugins/home/public';

import { useGetPackages, useLink, useCapabilities } from '../../hooks';
import { pkgKeyFromPackageInfo } from '../../services/pkg_key_from_package_info';

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

import React, { memo } from 'react';
import { EuiLink, EuiLinkAnchorProps } from '@elastic/eui';

import { useLink } from '../hooks';
import { AGENT_SAVED_OBJECT_TYPE } from '../constants';

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

import React from 'react';
import { EuiIcon, EuiIconProps } from '@elastic/eui';

import { usePackageIconType, UsePackageIconType } from '../hooks';

export const PackageIcon: React.FunctionComponent<
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
*/

import React, { useState, useEffect, useMemo } from 'react';

import {
QueryStringInput,
IFieldType,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import {
import { FormattedMessage } from '@kbn/i18n/react';
import { EuiText } from '@elastic/eui';
import { safeLoad } from 'js-yaml';

import {
useComboInput,
useStartServices,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,9 @@

import { i18n } from '@kbn/i18n';
import { ChromeBreadcrumb } from 'src/core/public';

import { BASE_PATH, Page, DynamicPagePathValues, pagePathGetters } from '../constants';

import { useStartServices } from './use_core';

const BASE_BREADCRUMB: ChromeBreadcrumb = {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
*/

import React, { useContext } from 'react';

import type { FleetConfigType } from '../../../plugin';

export const ConfigContext = React.createContext<FleetConfigType | null>(null);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,11 @@
*/

import React, { useState, useContext, useEffect } from 'react';

import { GetFleetStatusResponse } from '../types';

import { useConfig } from './use_config';
import { sendGetFleetStatus } from './use_request';
import { GetFleetStatusResponse } from '../types';

interface FleetStatusState {
enabled: boolean;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import React, { memo, useContext, useMemo } from 'react';
import { AppMountParameters } from 'kibana/public';
import { useLocation } from 'react-router-dom';

import { AnyIntraAppRouteState } from '../types';

interface IntraAppState<S extends AnyIntraAppRouteState = AnyIntraAppRouteState> {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

import { BASE_PATH, pagePathGetters } from '../constants';
import type { StaticPage, DynamicPage, DynamicPagePathValues } from '../constants';

import { useStartServices } from './';

const getPath = (page: StaticPage | DynamicPage, values: DynamicPagePathValues = {}): string => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,10 @@

import { useEffect, useState } from 'react';
import { ICON_TYPES } from '@elastic/eui';

import type { PackageInfo, PackageListItem } from '../types';
import { useLinks } from '../sections/epm/hooks';

import { sendGetPackageInfoByKey } from './index';

type Package = PackageInfo | PackageListItem;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,8 @@
* 2.0.
*/

import { useRequest, sendRequest, useConditionalRequest } from './use_request';
import type { SendConditionalRequestConfig } from './use_request';
import { agentPolicyRouteService } from '../../services';

import type {
GetAgentPoliciesRequest,
GetAgentPoliciesResponse,
Expand All @@ -23,6 +22,9 @@ import type {
DeleteAgentPolicyResponse,
} from '../../types';

import { useRequest, sendRequest, useConditionalRequest } from './use_request';
import type { SendConditionalRequestConfig } from './use_request';

export const useGetAgentPolicies = (query?: GetAgentPoliciesRequest['query']) => {
return useRequest<GetAgentPoliciesResponse>({
path: agentPolicyRouteService.getListPath(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,8 @@
* 2.0.
*/

import { useRequest, sendRequest } from './use_request';
import type { UseRequestConfig } from './use_request';
import { agentRouteService } from '../../services';

import type {
GetOneAgentResponse,
GetOneAgentEventsResponse,
Expand All @@ -32,6 +31,9 @@ import type {
PostNewAgentActionResponse,
} from '../../types';

import { useRequest, sendRequest } from './use_request';
import type { UseRequestConfig } from './use_request';

type RequestOptions = Pick<Partial<UseRequestConfig>, 'pollIntervalMs'>;

export function useGetOneAgent(agentId: string, options?: RequestOptions) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,11 @@
* 2.0.
*/

import { sendRequest } from './use_request';
import { appRoutesService } from '../../services';
import type { CheckPermissionsResponse } from '../../types';

import { sendRequest } from './use_request';

export const sendGetPermissionsCheck = () => {
return sendRequest<CheckPermissionsResponse>({
path: appRoutesService.getCheckPermissionsPath(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,11 @@
* 2.0.
*/

import { useRequest } from './use_request';
import { dataStreamRouteService } from '../../services';
import type { GetDataStreamsResponse } from '../../types';

import { useRequest } from './use_request';

export const useGetDataStreams = () => {
return useRequest<GetDataStreamsResponse>({
path: dataStreamRouteService.getListPath(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,17 @@
* 2.0.
*/

import { useRequest, sendRequest, useConditionalRequest } from './use_request';
import type { UseRequestConfig, SendConditionalRequestConfig } from './use_request';
import { enrollmentAPIKeyRouteService } from '../../services';

import type {
GetOneEnrollmentAPIKeyResponse,
GetEnrollmentAPIKeysResponse,
GetEnrollmentAPIKeysRequest,
} from '../../types';

import { useRequest, sendRequest, useConditionalRequest } from './use_request';
import type { UseRequestConfig, SendConditionalRequestConfig } from './use_request';

type RequestOptions = Pick<Partial<UseRequestConfig>, 'pollIntervalMs'>;

export function useGetOneEnrollmentAPIKey(keyId: string | undefined) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
* 2.0.
*/

import { useRequest, sendRequest } from './use_request';
import { epmRouteService } from '../../services';
import type {
GetCategoriesRequest,
Expand All @@ -19,6 +18,8 @@ import type {
} from '../../types';
import type { GetStatsResponse } from '../../../../../common';

import { useRequest, sendRequest } from './use_request';

export const useGetCategories = (query: GetCategoriesRequest['query'] = {}) => {
return useRequest<GetCategoriesResponse>({
path: epmRouteService.getCategoriesPath(),
Expand Down
Loading

0 comments on commit 97ccf1e

Please sign in to comment.