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

[Enterprise Search] Add eslint import/order rules #90530

Merged
merged 9 commits into from
Feb 9, 2021

Conversation

cee-chen
Copy link
Contributor

@cee-chen cee-chen commented Feb 5, 2021

Summary

Thanks to @byronhulcher's initiative, we should now have better living through automation:

  • VSCode will automatically alert for import order issues
  • Run node scripts/eslint x-pack/plugins/enterprise_search --fix (from kibana root) or update your IDE to auto-fix on save

Here's a quick list of what we now enforce:

  • Order, generally following external/absolute -> internal/relative ordering
  • Alphabetical order (within order groups)
  • Newlines between each order group. You can optionally sub-organize within a group with extra newlines.

An example set of imports:

/**
 * License header
 */

// All mock files (live within a `__mock__` folder, or direct `./something.mock` imports) should come first,
// to ensure mocks work as expected & avoid potential edge cases with mock hoisting
import { someSharedMock } from '../../../../__mocks__';
import { someLocalMock } from './some.mock';

// We created a rule to pull React out to the top of external imports as a standard for React component views
// Note that this in combination with our newline rule makes it so there's newlines between React + other externals :(
import React, { useEffect } from 'react';
import { Route } from 'react-router-dom';

// All other `external` imports
import { shallow } from 'enzyme';
import { useValues } from 'kea';
import { get } from 'lodash';

// `internal` (a.k.a Elastic) imports
import { EuiButton } from '@elastic/eui';
import { i18n } from '@kbn/i18n';
import { FormattedMessage } from '@kbn/i18n/react';

// `parent` imports
import { AnotherHelper }  '../../../shared/helpers/another';
import { SomeHelper } from '../../../shared/helpers/some';
import { RandomParent } from '../parent';

// `sibling` imports
import { SomeConstant } from './constants';
import { SomeSiblingComponent } from './some_sibling';
import { SomeType } from './types';

// `index` import
import { ThisComponent } from './';

The number of lines changed is probably too many to reasonably request a review for, so feel free to skim sections that only apply to you (e.g. WS/AS teams), or follow by-commit.

Resources used during PR

Checklist

  • Linting passes
  • All unit tests pass as before
  • Kibana is able to start (no errors with imports)

@cee-chen cee-chen added Feature:Plugins release_note:skip Skip the PR/issue when compiling release notes v7.12.0 labels Feb 5, 2021
@cee-chen cee-chen requested review from scottybollinger, byronhulcher and a team February 5, 2021 22:36
Comment on lines +1128 to +1129
pattern:
'{../../../../../../,../../../../../,../../../../,../../../,../../,../}{common/,*}__mocks__{*,/**}',
Copy link
Contributor Author

@cee-chen cee-chen Feb 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes this is absolutely horrifying. No there is no way else to do this. You would think that **/__mocks__/** would work. It absolutely does not for whatever reason (.. doesn't count as **).

See: import-js/eslint-plugin-import#1632

So yeah, I guess let's hope we don't go 7 folders in deep from a __mocks__ file, or we're adding yet another another , of 7x ../s :dead_inside:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Y'all are also free to test this yourselves (glob tester links are in the PR description, under resources) but I can also reassure you I spent several hours of my life yesterday and today trying to get this pathGroup work and this was the only way it did.

group: 'unknown',
},
{
pattern: 'react*',
Copy link
Contributor Author

@cee-chen cee-chen Feb 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opted to group React-adjacent imports (e.g. react-router-dom and react-redux) in with React itself. Alphabetizing wise React should still remain at the top.

I don't feel super strongly if people would rather have only the main react lib in this group, feel free to comment if you do.

group: 'internal',
},
],
pathGroupsExcludedImportTypes: [],
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is required for the React external rule to work. See import-js/eslint-plugin-import#1874

pathGroupsExcludedImportTypes: [],
alphabetize: {
order: 'asc',
caseInsensitive: true,
Copy link
Contributor Author

@cee-chen cee-chen Feb 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I listed caseInsensitive as a reason to upgrade from eslint-plugin-import v2.19 to v2.22, then I realized Kibana enforces snake_case on files anyway so it's not doing much 🤦‍♀️

I left it in anyway as I think we would still probably want this rule if we were on the old ent-search repo with PascalCased component filenames, but would be curious to hear what others think and if my assumption was wrong there

order: 'asc',
caseInsensitive: true,
},
'newlines-between': 'always-and-inside-groups',
Copy link
Contributor Author

@cee-chen cee-chen Feb 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Docs link) I opted for always-and-inside-groups instead of always because I wanted the option to sub-organize within groups for large amount of imports. Mock example:

import { SharedHelperA } from '../../../shared/a'
import { SharedHelperB } from '../../../shared/b'
import { SharedHelperC } from '../../../shared/c'

import { ProductSpecificHelperA } from '../app_search/utils/util_a';
import { ProductSpecificHelperB } from '../app_search/utils/util_b';

Actual example of newlines within a group being used to make code more readable in our server/plugin.ts file:

import { registerTelemetryUsageCollector as registerASTelemetryUsageCollector } from './collectors/app_search/telemetry';
import { registerTelemetryUsageCollector as registerESTelemetryUsageCollector } from './collectors/enterprise_search/telemetry';
import { registerTelemetryUsageCollector as registerWSTelemetryUsageCollector } from './collectors/workplace_search/telemetry';

import { checkAccess } from './lib/check_access';
import {
  EnterpriseSearchRequestHandler,
  IEnterpriseSearchRequestHandler,
} from './lib/enterprise_search_request_handler';

import { registerAppSearchRoutes } from './routes/app_search';
import { registerConfigDataRoute } from './routes/enterprise_search/config_data';
import { registerTelemetryRoute } from './routes/enterprise_search/telemetry';
import { registerWorkplaceSearchRoutes } from './routes/workplace_search';

import { appSearchTelemetryType } from './saved_objects/app_search/telemetry';
import { enterpriseSearchTelemetryType } from './saved_objects/enterprise_search/telemetry';
import { workplaceSearchTelemetryType } from './saved_objects/workplace_search/telemetry';

@cee-chen cee-chen force-pushed the eslint-3 branch 2 times, most recently from ec15136 to b59c964 Compare February 8, 2021 17:14
- newlines between each group
- mocks in test files before everything else
- React before all other externals
- these appear to be mostly due to jest.mock() or const mixed in with imports, which confuses the autofixer
+ some manual fixes/tweaks along the way
- some opinionated changes, particularly around IFlashMessages and grouping together types coming from Kibana (src->../../src)
- Some opinionated changes (src/core -> ../../src/core) to keep types/mocks together
- same opinionated src->../src changes to keep deps grouped together

- opinionated require->import fetch change in enterprise_esarch_config_api.test.ts

- opinionated [] inclusion of builtins & external imports together (mostly for enterprise_search_request_handler.ts)
@cee-chen
Copy link
Contributor Author

cee-chen commented Feb 8, 2021

Tests should be fixed now, really dumb mistake on my part. @elastic/enterprise-search-frontend would appreciate a review whenever, but as a heads up / reminder this will need to be rebased into existing/open PRs to prevent lint errors on master.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
enterpriseSearch 1190 1191 +1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
enterpriseSearch 1.8MB 1.8MB +54.0B

History

  • 💔 Build #104808 failed b59c964c3a9e5ec25e3befac014f39eeea12202f
  • 💔 Build #104462 failed ec15136710e3919f26e356fe57a6f827d1332931
  • 💔 Build #104457 failed 9132d60489e2ad1da5c49476222aa347eddd4b1d

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

Copy link
Contributor

@scottybollinger scottybollinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@cee-chen
Copy link
Contributor Author

cee-chen commented Feb 9, 2021

🚀

@cee-chen cee-chen merged commit f339962 into elastic:master Feb 9, 2021
@cee-chen cee-chen deleted the eslint-3 branch February 9, 2021 17:33
cee-chen pushed a commit to cee-chen/kibana that referenced this pull request Feb 9, 2021
* Add import rules

- newlines between each group
- mocks in test files before everything else
- React before all other externals

* Run --fix on public/applications/workplace_search

* Manually fix errors still present in WS public files

- these appear to be mostly due to jest.mock() or const mixed in with imports, which confuses the autofixer

* Run --fix on public/applications/app_search

+ some manual fixes/tweaks along the way

* Run --fix on public/applications/shared

- some opinionated changes, particularly around IFlashMessages and grouping together types coming from Kibana (src->../../src)

* Run --fix on public/applications/enterprise_search

- mostly straightforward

* Run --fix on public/applications/__mocks__

* Fix remaining top-level public files

- Some opinionated changes (src/core -> ../../src/core) to keep types/mocks together

* Run --fix on server/ files

- same opinionated src->../src changes to keep deps grouped together

- opinionated require->import fetch change in enterprise_esarch_config_api.test.ts

- opinionated [] inclusion of builtins & external imports together (mostly for enterprise_search_request_handler.ts)
cee-chen pushed a commit that referenced this pull request Feb 9, 2021
)

* [Enterprise Search] Add eslint import/order rules (#90530)

* Add import rules

- newlines between each group
- mocks in test files before everything else
- React before all other externals

* Run --fix on public/applications/workplace_search

* Manually fix errors still present in WS public files

- these appear to be mostly due to jest.mock() or const mixed in with imports, which confuses the autofixer

* Run --fix on public/applications/app_search

+ some manual fixes/tweaks along the way

* Run --fix on public/applications/shared

- some opinionated changes, particularly around IFlashMessages and grouping together types coming from Kibana (src->../../src)

* Run --fix on public/applications/enterprise_search

- mostly straightforward

* Run --fix on public/applications/__mocks__

* Fix remaining top-level public files

- Some opinionated changes (src/core -> ../../src/core) to keep types/mocks together

* Run --fix on server/ files

- same opinionated src->../src changes to keep deps grouped together

- opinionated require->import fetch change in enterprise_esarch_config_api.test.ts

- opinionated [] inclusion of builtins & external imports together (mostly for enterprise_search_request_handler.ts)

* Fix failures
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Plugins release_note:skip Skip the PR/issue when compiling release notes v7.12.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants