Add absolute path import handling in kibana #52995

pgayvallet · 2019-12-13T15:08:09Z

Add absolute path imports support to Kibana

// x-pack/plugins/my-plugin/someplace
import { actualConcreteType } from 'src/core/server/somewhere'

Fix #40446

This currently only supports src and x-pack, but adding additional root folders is (almost) trivial if we want to.

This is widely based on @spalger work in #49766

Note: I want to keep this specific PR history clean, so I will use rebase strategy instead of merge to reintegrate master changes.

PR does the following:

Add a new (internal) babel plugin to our kbn-babel-preset to rewrite absolute imports to relative ones during babel transpilation
Modify the kbn-eslint-import-resolver-kibana to allow eslint to understand and resolves these absolute imports
Edit jest configurations to properly handles absolute imports

PR does not:

Add eslint rules to enforce (or fix) using absolute paths instead of relative ones for long (aka ../../../../../../[...] imports. This will ideally be done in a future PR as this means massive code edition for the auto-fix.

Note The last commit is a proof of concept on actual usages and will be reverted.

Open questions / discussions

the `import/order` rule consider that absolute imports should be done before relative ones

For example this is a violation:

import { INDEX_PATTERN_ELASTICSEARCH } from '../../common/constants';
import { ClusterDetails } from 'src/legacy/core_plugins/telemetry/server/collection_manager';

Does that makes sense to us, or should we add exception (if that's possible) for imports from our root paths?

Our `module_migrations` rules forbid x-pack absolute imports.

I.E is a violation:

// x-pack/plugins/licensing/server/licensing_route_handler_context.test.ts
import { licenseMock } from 'x-pack/plugins/licensing/common/licensing.mock';

When I look at the actual rule implementation, there is a comment that makes me think this can safely be removed, but I would like confirmation for that:

  // support for toRelative added to migrate away from X-Pack being bundled
  // within node modules. after that migration, this can be removed.

The eslint resolver modifications 'fixed' (?) the `import/order` rule in x-pack

This was previously allowed in x-pack:

import { UsageCollectionSetup } from 'src/plugins/usage_collection/server';
import { ObjectType } from '@kbn/config-schema';

Which is now considered violations (see CI) as eslint properly recognize src/... as local sources instead of a module. I can just autofix these in these PR, but once again, would like confirmation on that.

elasticmachine · 2019-12-16T15:16:19Z

Pinging @elastic/kibana-platform (Team:Platform)

elasticmachine · 2019-12-16T15:16:26Z

Pinging @elastic/kibana-operations (Team:Operations)

pgayvallet · 2019-12-16T15:18:00Z

packages/eslint-config-kibana/typescript.js

-        react: {
-          version: semver.valid(semver.coerce(PKG.dependencies.react)),
-        },


This was redundant as already present in packages/eslint-config-kibana/react.js

pgayvallet · 2019-12-16T15:20:50Z

packages/kbn-eslint-plugin-eslint/rules/no_restricted_paths.js

-      const absoluteImportPath = importPath[0] === '.' ? resolve(importPath, context) : undefined;
+      const absoluteImportPath = resolve(importPath, context);


paths not starting with . can be local now

pgayvallet · 2019-12-16T15:22:44Z

src/core/server/config/raw_config_service.test.mocks.ts

-jest.mock('./read_config', () => ({
+jest.mock('src/core/server/config/read_config', () => ({
  getConfigFromFiles: mockGetConfigFromFiles,
 }));


(this commit will be deleted) This works even if read_config is imported using relative imports in test or sources import XXX from '../foo/read_config' (jest uses the resolved absolute path)

…r from source + cleanup

… and jest.doMock

elasticmachine · 2019-12-16T17:56:52Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 78e271a

History

💔 Build #15737 failed 4fa90af
💔 Build #15686 failed f03a90a
💚 Build #15553 succeeded daf9fcd

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

mshustov · 2019-12-17T14:21:49Z

ack: will review tonight

mshustov · 2019-12-17T16:49:14Z

packages/kbn-babel-preset/transforms/rewrite_absolute_imports.js

+        sourceFileAbsPath = sourceFileName;
+      } else if (filename && Path.isAbsolute(filename)) {
+        sourceFileAbsPath = filename;
+      } else if (!sourceFileAbsPath && root) {


nit: !sourceFileAbsPath can be remove

mshustov · 2019-12-17T16:50:32Z

packages/kbn-babel-preset/transforms/rewrite_absolute_imports.js

+      } else if (!sourceFileAbsPath && root) {
+        sourceFileAbsPath = Path.resolve(root, sourceFileName || filename);
+      } else {
+        throw new Error(`


shouldn't this check done after all if/else branches? for cases when https://github.com/elastic/kibana/pull/52995/files#diff-0b4873344bd872a80014131ef6a02a27R73 fails

mshustov · 2019-12-17T17:16:37Z

packages/kbn-eslint-import-resolver-kibana/import_resolver_kibana.js

@@ -66,9 +68,20 @@ function tryNodeResolver(importRequest, file, config) {
  );
 }

+function isImportedFromRoot(importRequest) {


As I understand packages/kbn-eslint-plugin-eslint/rules/no_restricted_paths.js should integrate this resolution logic as well. Can we extract it in a common folder to share? Can be done in a separate PR.

mshustov · 2019-12-17T17:45:10Z

tsconfig.json

@@ -3,6 +3,8 @@
    "baseUrl": ".",
    "paths": {
      // Allows for importing from `kibana` package for the exported types.
+      "src/*": ["src/*"],


shouldn't we update x-pack tsconfig as well?
update: as I can see mapping from x-pack works without adding these paths(due to baseUrl set to the root folder probably?)

mshustov · 2019-12-17T18:04:03Z

x-pack/legacy/plugins/monitoring/server/telemetry_collection/get_cluster_uuids.ts


 import {
  ClusterDetailsGetter,
  StatsCollectionConfig,
  ClusterDetails,
-} from '../../../../../../src/legacy/core_plugins/telemetry/server/collection_manager';
+} from 'src/legacy/core_plugins/telemetry/server/collection_manager';


as I understand this is handled by built-in ts functionality. could you add an example for js code to make sure that server runs?

mshustov · 2019-12-17T18:06:42Z

packages/kbn-eslint-plugin-eslint/package.json

@@ -8,6 +8,7 @@
    "babel-eslint": "^10.0.3"
  },
  "dependencies": {
+    "@kbn/dev-utils": "1.0.0",


is this used?

spalger · 2020-06-03T17:06:37Z

Sorry, this fell off my radar. Is this still something you want to do? If so, please get it passing again and re-request review. Thanks!

pgayvallet · 2020-06-03T19:15:23Z

Sorry, this fell off my radar. Is this still something you want to do? If so, please get it passing again and re-request review. Thanks!

Given the team's load, it's not on top of my priority list atm. But hopefully this will be done this year.

spalger · 2020-10-15T16:04:51Z

@pgayvallet I'm going to close this now and we can reopen if we feel like it's necessary.

pgayvallet force-pushed the kbn-40446-abs-imports branch from 5cd2eff to 6d6d380 Compare December 13, 2019 17:17

elastic deleted a comment from elasticmachine Dec 13, 2019

pgayvallet force-pushed the kbn-40446-abs-imports branch from 02e2168 to daf9fcd Compare December 13, 2019 22:32

pgayvallet mentioned this pull request Dec 16, 2019

Allow custom new platform plugin paths in integration tests #49927

Closed

pgayvallet force-pushed the kbn-40446-abs-imports branch from daf9fcd to f03a90a Compare December 16, 2019 10:45

pgayvallet changed the title ~~absolute path handling~~ Add absolute path import handling in kibana Dec 16, 2019

pgayvallet added the Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc label Dec 16, 2019

pgayvallet added the Team:Operations Team label for Operations Team label Dec 16, 2019

pgayvallet commented Dec 16, 2019

View reviewed changes

pgayvallet requested review from epixa, spalger and a team December 16, 2019 15:25

pgayvallet marked this pull request as ready for review December 16, 2019 15:25

pgayvallet requested a review from a team as a code owner December 16, 2019 15:25

pgayvallet added release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. v7.7.0 v8.0.0 labels Dec 16, 2019

pgayvallet added 5 commits December 16, 2019 17:24

Add abs path rewriting to kbn babel preset

ad50367

Change eslint config to handle absolute paths imports

63d4787

fix rewrite_absolute_imports when importing from same or nested folde…

1065782

…r from source + cleanup

Add abs import support in jest

b70abcc

(will be reverted) proof examples is src, x-pack, and using jest.mock…

78e271a

… and jest.doMock

pgayvallet force-pushed the kbn-40446-abs-imports branch from 4fa90af to 78e271a Compare December 16, 2019 16:27

pgayvallet mentioned this pull request Dec 16, 2019

An alternative to relative imports for local source code #40446

Closed

mshustov reviewed Dec 17, 2019

View reviewed changes

lukeelmers mentioned this pull request Feb 11, 2020

[ui/utils/query_string]: Remove unused methods & migrate apps to querystring lib #56957

Merged

7 tasks

lizozom mentioned this pull request Feb 27, 2020

src/core/server vs kibana/server #58712

Closed

mshustov removed the v7.7.0 label May 10, 2020

joshdover mentioned this pull request May 11, 2020

Kibana paths configuration elastic/elastic-stack-installers#63

Open

spalger removed their request for review June 3, 2020 17:06

spalger marked this pull request as draft October 15, 2020 16:04

spalger closed this Oct 15, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add absolute path import handling in kibana #52995

Add absolute path import handling in kibana #52995

pgayvallet commented Dec 13, 2019 •

edited

Loading

elasticmachine commented Dec 16, 2019

elasticmachine commented Dec 16, 2019

pgayvallet Dec 16, 2019

pgayvallet Dec 16, 2019

pgayvallet Dec 16, 2019

elasticmachine commented Dec 16, 2019

mshustov commented Dec 17, 2019

mshustov Dec 17, 2019

mshustov Dec 17, 2019

mshustov Dec 17, 2019

mshustov Dec 17, 2019 •

edited

Loading

mshustov Dec 17, 2019

mshustov Dec 17, 2019

spalger commented Jun 3, 2020

pgayvallet commented Jun 3, 2020

spalger commented Oct 15, 2020

		const absoluteImportPath = importPath[0] === '.' ? resolve(importPath, context) : undefined;
		const absoluteImportPath = resolve(importPath, context);

Add absolute path import handling in kibana #52995

Add absolute path import handling in kibana #52995

Conversation

pgayvallet commented Dec 13, 2019 • edited Loading

Add absolute path imports support to Kibana

Open questions / discussions

the import/order rule consider that absolute imports should be done before relative ones

Our module_migrations rules forbid x-pack absolute imports.

The eslint resolver modifications 'fixed' (?) the import/order rule in x-pack

elasticmachine commented Dec 16, 2019

elasticmachine commented Dec 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Dec 16, 2019

💔 Build Failed

History

mshustov commented Dec 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mshustov Dec 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spalger commented Jun 3, 2020

pgayvallet commented Jun 3, 2020

spalger commented Oct 15, 2020

pgayvallet commented Dec 13, 2019 •

edited

Loading

the `import/order` rule consider that absolute imports should be done before relative ones

Our `module_migrations` rules forbid x-pack absolute imports.

The eslint resolver modifications 'fixed' (?) the `import/order` rule in x-pack

mshustov Dec 17, 2019 •

edited

Loading