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

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

Closed
mshustov opened this issue Mar 18, 2019 · 1 comment
Closed

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

mshustov opened this issue Mar 18, 2019 · 1 comment
Assignees
Labels
Feature:New Platform Meta non-issue Indicates to automation that a pull request should not appear in the release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc

Comments

@mshustov
Copy link
Contributor

mshustov commented Mar 18, 2019

Problem

Client and side parts of the core and plugins should expose their contracts explicitly via index.js files.
Direct imports from core/server/foo/..., core/public/foo/..., src/plugins/bar/public/..., src/plugins/bar/server/... should be restricted by linter.

Considered options

  • use linter. There are 2 types of files in codebase: js & ts, we have to setup linters for both. Disadvantages: setup twice and manual synchronization (until tslint merges into eslint this year)
  • use a separate tool for check - webpack plugin or CLI utility. Disadvantages: The check is run on late stage, on CI or as a pre-commit hook.

I decided to go with the former option as providing better DX.

Requirements

Linter should block attempts to import from module internals. This way we force a module to declare public interface explicitly.
Linter should check only product code, because quite often in tests and dev-utils we want to manipulate module internals, mock a part of a module, for example.

eslint

PR #33697
import/no-restricted-paths uses a similar approach with zones.

Restrict import from sibling folders

Now src/plugins/foo/foo.js can import from src/plugins/bar/bar.js. Such restriction is not supported by import/no-restricted-paths, will be implemented as a separate plugin.

tslint

PR: #34688

PR on hold, waits migration from tslint to eslint #33562

I start with tslint setup. There is import-blacklist rule, which implemented in the last version.

  import-blacklist:
    - true
    -
      - "core/public/(?!utils)."
      - "core/server/(?!utils)."
      - "^(?!x-pack).*\/plugins.*server\/"
      - "^(?!x-pack).*\/plugins.*public\/."

The problem with it this approach that restrictions are applied for the whole project, we cannot exclude or override settings for specific path and this feature won't be implemented palantir/tslint#3447
Also, because it's simple string check without file path resolution, we cannot limit access when import expression doesn't contain the full path(say one plugin import internals of another from sibling folder import * from '../sibling_folder/server/types.ts.

I decided to go with https://github.com/vladimiry/tslint-rules-bunch as this rule provides granular setup for a specific path with glob syntax and uses file path resolution mechanism internally.

Related issues

Export types from root of systems
#33963

@mshustov mshustov created this issue from a note in New platform cleanup (To do) Mar 18, 2019
@mshustov mshustov self-assigned this Mar 18, 2019
@mshustov mshustov moved this from To do to In progress in New platform cleanup Mar 18, 2019
@mshustov mshustov added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Meta labels Mar 20, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform

@mshustov mshustov added the non-issue Indicates to automation that a pull request should not appear in the release notes label Mar 20, 2019
@mshustov mshustov moved this from In progress to Done in New platform cleanup Apr 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:New Platform Meta non-issue Indicates to automation that a pull request should not appear in the release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc
Projects
No open projects
Development

No branches or pull requests

2 participants