[New platform] Restrict import internals of plugins & core services in ts files #33562

mshustov · 2019-03-20T10:38:55Z

Summary

Issue #33444
The full description is in the issue.
Added import-zones rule for tslint restricting import from core and plugins sub-folders.
Although, import restriction shouldn't be applied to the same folder, descendent folders, so I have to enhance import-zones with this functionality. Also we are in process of migration from tslint to eslint, until this process is done I put the PR on hold. Afterwards this plugin will be ported to eslint

Blocked by #33826

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

~~[ ] This was checked for cross-browser compatibility, including a check against IE11~~
~~[ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support~~
~~[ ] Documentation was added for features that require explanation or tutorials~~
~~[ ] Unit or functional tests were updated or added to match the most common scenarios~~
~~[ ] This was checked for keyboard-only and screenreader accessibility~~

For maintainers

~~[ ] This was checked for breaking API changes and was labeled appropriately~~
~~[ ] This includes a feature addition or change that requires a release note and was labeled appropriately~~

elasticmachine · 2019-03-20T10:38:57Z

Pinging @elastic/kibana-platform

src/legacy/ui/public/chrome/api/theme.ts

src/core/public/index.ts

mshustov · 2019-03-20T10:59:57Z

src/legacy/server/saved_objects/routes/_mock_server.ts

@@ -18,6 +18,7 @@
 */

 import Hapi from 'hapi';
+// tslint:disable-next-line:no-import-zones


shouldn't we provide server factory / builder for testing purposes?
similar test https://github.com/elastic/kibana/blob/master/x-pack/plugins/security/server/routes/api/public/roles/put.test.js

rudolf

no-import-zones seems really powerful 👍

I've made some comments about how we choose to expose types, but that's perhaps a meta issues that doesn't have to block this PR.

src/legacy/ui/public/chrome/api/theme.ts

src/core/public/index.ts

mshustov · 2019-03-20T15:40:27Z

I just realised that plugins still can import from sibling folders.
the current setup:

  - target: src/plugins/**/public/**/*
    from:
      - src/core/server/**/*
      - src/core/public/**/*
      - src/plugins/**/server/**/*

src/plugins/a/public/internal.ts can import from src/plugins/b/public/internal.ts 😞
What we want here - import restriction shouldn't be applied to the same folder, descendent folders
I will take a look if I can enhance tslint plugin, because I don't want to write custom one.

mshustov · 2019-03-21T10:43:17Z

src/dev/tslint/rules/noImportZonesRule.js

+const { findImports, ImportKind } = require('tsutils');
+
+function traverseToTopFolder(src, pattern) {
+  while(src && mm([src], pattern).length) {


I think it's okay to have a local fork of the no-import-zones rule, we can port some logic to eslint as well (if we want ofc).
the only problem that I see right now - we don't have tests and it's not in written in ts anymore.

mshustov · 2019-03-21T12:21:24Z

src/legacy/ui/public/chrome/api/injected_vars.ts


-let newPlatformInjectedVars: InjectedMetadataStart;
+let newPlatformInjectedVars: InjectedMetadata.InjectedMetadataStart;


I'm fine with having domain name as a prefix for start contract, it's useful for manual search

mshustov · 2019-03-21T12:32:37Z

src/core/public/injected_metadata/index.ts

-  InjectedMetadataParams,
-  InjectedMetadataStart,
-} from './injected_metadata_service';
+export * from './injected_metadata_service';


I made changes to improve "Go to definition" work. Otherwise, ts resolves this place as a definition. Potential problem: we don't see what file exports. Sometimes it can export some functionality only for testing, which is also bad imho (because it's not public interface).

mshustov · 2019-03-21T13:53:32Z

src/dev/tslint/rules/noImportZonesRule.js

+ * under the License.
+ */
+
+/* @notice


What should we do when use software licensed under MIT? Included a note as per https://github.com/elastic/kibana/blob/master/webpackShims/childnode-remove-polyfill.js

This is how we comply with The above copyright notice and this permission notice shall be included in all copies or substantial portions of the Software., but you should include /* eslint-disable @kbn/license-header/require-license-header */ too, since this isn't Apache 2.0 code.

since this isn't Apache 2.0 code.

you mean only this file is not Apache 2.0 anymore because of MIT license? or it's not needed to be licensed under Apache 2.0 at all?

you mean only this file is not Apache 2.0 anymore because of MIT license?

correct

elasticmachine · 2019-03-21T15:33:30Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

eliperelman · 2019-03-21T16:21:01Z

Adding @elastic/kibana-operations to evaluate tslint rule.

mistic · 2019-03-21T17:37:13Z

@restrry I'm working on this branch https://github.com/mistic/kibana/tree/migrate-from-tslint
in order to move away from tslint to eslint over all the project and it is almost done. I'm just working in the last part of solving some rules problems. Do you think it is viable to port that rule to an eslint based one and hold this PR until we move ?

mshustov · 2019-03-21T20:00:41Z

@mistic ok, sounds good 👍
would you mind pinging me when you are ready for review and I can start porting rule to eslint?
also may I ask you sent a draft PR? I can declare explicitly that the current one is blocked by it.

mistic · 2019-03-21T20:56:53Z

@restrry awesome! I'll let you know as soon as I feel we are ready to start porting this 😃

mshustov · 2019-03-29T08:01:44Z

I close PR. when #33826 is merged we need to check eslint lints *ts. files as well or enable such check

mshustov added 3 commits March 20, 2019 10:45

add import-zones dependency

517a359

configure import zones

725c8b0

change imports, fix errors

c87139e

mshustov added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:New Platform v7.2.0 labels Mar 20, 2019

mshustov requested a review from a team March 20, 2019 10:38

mshustov commented Mar 20, 2019

View reviewed changes

src/legacy/ui/public/chrome/api/theme.ts Outdated Show resolved Hide resolved

mshustov commented Mar 20, 2019

View reviewed changes

src/core/public/index.ts Outdated Show resolved Hide resolved

mshustov commented Mar 20, 2019

View reviewed changes

mshustov added the review label Mar 20, 2019

mshustov mentioned this pull request Mar 20, 2019

Lint for cross-system imports only happening to root of systems #33444

Closed

This comment has been minimized.

Sign in to view

rudolf approved these changes Mar 20, 2019

View reviewed changes

src/legacy/ui/public/chrome/api/theme.ts Outdated Show resolved Hide resolved

src/core/public/index.ts Outdated Show resolved Hide resolved

mshustov added the non-issue Indicates to automation that a pull request should not appear in the release notes label Mar 20, 2019

mshustov commented Mar 21, 2019

View reviewed changes

mshustov removed the review label Mar 21, 2019

mshustov changed the title ~~[New platform] Restrict import internals of plugins & core services in ts files~~ WIP [New platform] Restrict import internals of plugins & core services in ts files Mar 21, 2019

This comment has been minimized.

Sign in to view

mshustov force-pushed the issue-33444-restrict-imports-tslint branch 2 times, most recently from 24eb4c7 to d939b45 Compare March 21, 2019 12:17

mshustov changed the title ~~WIP [New platform] Restrict import internals of plugins & core services in ts files~~ [New platform] Restrict import internals of plugins & core services in ts files Mar 21, 2019

mshustov added the review label Mar 21, 2019

mshustov commented Mar 21, 2019

View reviewed changes

mshustov requested a review from rudolf March 21, 2019 12:22

mshustov commented Mar 21, 2019

View reviewed changes

This comment has been minimized.

Sign in to view

make a local copy of no-import-zones

20f4aba

mshustov added 2 commits March 21, 2019 14:50

add the same or descendent folder check

58b7994

export types with namespaces

2fbfd00

mshustov force-pushed the issue-33444-restrict-imports-tslint branch from d939b45 to 2fbfd00 Compare March 21, 2019 13:51

mshustov commented Mar 21, 2019

View reviewed changes

This comment has been minimized.

Sign in to view

update NOTICE.txt

8f29599

mshustov requested a review from a team March 21, 2019 15:38

eliperelman requested a review from a team March 21, 2019 16:20

mshustov added the blocked label Mar 21, 2019

mshustov mentioned this pull request Mar 22, 2019

[eslint] merge custom rules into a single plugin #33733

Merged

mshustov added release_note:skip Skip the PR/issue when compiling release notes and removed non-issue Indicates to automation that a pull request should not appear in the release notes labels Mar 26, 2019

mshustov mentioned this pull request Mar 27, 2019

[New platform] Export types from root levels #33973

Merged

3 tasks

mshustov closed this Mar 29, 2019

mshustov removed review blocked release_note:skip Skip the PR/issue when compiling release notes v7.2.0 labels Apr 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[New platform] Restrict import internals of plugins & core services in ts files #33562

[New platform] Restrict import internals of plugins & core services in ts files #33562

mshustov commented Mar 20, 2019 •

edited

Loading

elasticmachine commented Mar 20, 2019

mshustov Mar 20, 2019

This comment has been minimized.

rudolf left a comment

mshustov commented Mar 20, 2019 •

edited

Loading

mshustov Mar 21, 2019 •

edited

Loading

This comment has been minimized.

mshustov Mar 21, 2019

mshustov Mar 21, 2019 •

edited

Loading

This comment has been minimized.

mshustov Mar 21, 2019

spalger Mar 21, 2019

mshustov Mar 21, 2019 •

edited

Loading

spalger Mar 21, 2019 •

edited

Loading

This comment has been minimized.

elasticmachine commented Mar 21, 2019

eliperelman commented Mar 21, 2019

mistic commented Mar 21, 2019 •

edited

Loading

mshustov commented Mar 21, 2019 •

edited

Loading

mistic commented Mar 21, 2019

mshustov commented Mar 29, 2019


		let newPlatformInjectedVars: InjectedMetadataStart;
		let newPlatformInjectedVars: InjectedMetadata.InjectedMetadataStart;

[New platform] Restrict import internals of plugins & core services in ts files #33562

[New platform] Restrict import internals of plugins & core services in ts files #33562

Conversation

mshustov commented Mar 20, 2019 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented Mar 20, 2019

mshustov Mar 20, 2019

Choose a reason for hiding this comment

This comment has been minimized.

rudolf left a comment

Choose a reason for hiding this comment

mshustov commented Mar 20, 2019 • edited Loading

mshustov Mar 21, 2019 • edited Loading

Choose a reason for hiding this comment

This comment has been minimized.

mshustov Mar 21, 2019

Choose a reason for hiding this comment

mshustov Mar 21, 2019 • edited Loading

Choose a reason for hiding this comment

This comment has been minimized.

mshustov Mar 21, 2019

Choose a reason for hiding this comment

spalger Mar 21, 2019

Choose a reason for hiding this comment

mshustov Mar 21, 2019 • edited Loading

Choose a reason for hiding this comment

spalger Mar 21, 2019 • edited Loading

Choose a reason for hiding this comment

This comment has been minimized.

elasticmachine commented Mar 21, 2019

💚 Build Succeeded

eliperelman commented Mar 21, 2019

mistic commented Mar 21, 2019 • edited Loading

mshustov commented Mar 21, 2019 • edited Loading

mistic commented Mar 21, 2019

mshustov commented Mar 29, 2019

mshustov commented Mar 20, 2019 •

edited

Loading

mshustov commented Mar 20, 2019 •

edited

Loading

mshustov Mar 21, 2019 •

edited

Loading

mshustov Mar 21, 2019 •

edited

Loading

mshustov Mar 21, 2019 •

edited

Loading

spalger Mar 21, 2019 •

edited

Loading

mistic commented Mar 21, 2019 •

edited

Loading

mshustov commented Mar 21, 2019 •

edited

Loading