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

repo: add runtime-import-check eslint rule #10124

Merged
merged 1 commit into from Sep 21, 2021

Conversation

vince-fugnitto
Copy link
Member

What it does

Fixes: #5873

The pull-request adds a custom eslint rule meant to check for invalid runtime imports which do not comply to our code organization guidelines. The rule will help us identify uses of invalid imports which cause downstream adopters and extenders runtime issues.

How to test

The new rule should report errors when we use invalid imports based on different runtimes.

The pull-request includes a baseline commit meant to suppress the errors currently in the codebase so we can track and fix them, while new errors from contributions should fail linting.

Review checklist

Reminder for reviewers

Signed-off-by: vince-fugnitto vincent.fugnitto@ericsson.com

@vince-fugnitto vince-fugnitto added quality issues related to code and application quality linting issues related to linting labels Sep 16, 2021
@vince-fugnitto vince-fugnitto self-assigned this Sep 16, 2021
Copy link
Contributor

@alvsan09 alvsan09 left a comment

Choose a reason for hiding this comment

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

I have tested it by removing some of the lint disabling lines and it works as expected 👍

The code looks good to me, with only one typo mentioned as an inline comment.

dev-packages/eslint-plugin/rules/runtime-import-check.js Outdated Show resolved Hide resolved
@alvsan09
Copy link
Contributor

@vince-fugnitto , it would be great to raise an issue and make a reference to it in this PR i.e. to track fixes for the deviations disabled on this baseline.

Copy link
Contributor

@alvsan09 alvsan09 left a comment

Choose a reason for hiding this comment

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

The additional changes look good to me,
I have only verified the functionality under Linux.

@vince-fugnitto
Copy link
Member Author

@colin-grant-work the error message was updated to include the list of allowed folder as requested offline.

Example:

/home/evinfug/workspaces/theia/packages/core/src/electron-common/electron-main-window-service.ts
  17:34  error  '../browser/window/window-service' cannot be imported in 'electron-common', only 'electron-common, common' is allowed  @theia/runtime-import-check

/home/evinfug/workspaces/theia/packages/core/src/electron-main/electron-main-application.ts
  33:37  error  '../browser/window/window-service' cannot be imported in 'electron-main', only 'electron-main, electron-common, node, common' is allowed  @theia/runtime-import-check

/home/evinfug/workspaces/theia/packages/core/src/electron-main/electron-main-window-service-impl.ts
  23:34  error  '../browser/window/window-service' cannot be imported in 'electron-main', only 'electron-main, electron-common, node, common' is allowed  @theia/runtime-import-check

The commit adds a custom eslint rule to warn when we are importing from
folders meant for incompatible runtimes, and enforces our code
organization guidelines. When we import from such invalid runtimes we
may cause downstream adopters and extensions issues, namely when they
attempt to import from valid sources but our imports break them.

Signed-off-by: vince-fugnitto <vincent.fugnitto@ericsson.com>
Co-authored-by: Paul Maréchal <paul.marechal@ericsson.com>
@paul-marechal paul-marechal merged commit a73a746 into master Sep 21, 2021
@github-actions github-actions bot added this to the 1.18.0 milestone Sep 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
linting issues related to linting quality issues related to code and application quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tslint rule to enforce project organization
4 participants