feat(linter): automatic fixes for noRelativeOrAbsoluteImportsAcrossLibraries and noSelfCircularDependencies #8901

juristr · 2022-02-08T12:49:51Z

Current Behavior

Expected Behavior

adds a Lint rule fixer for automatically adjusting noRelativeOrAbsoluteImportsAcrossLibraries and noSelfCircularDependencies violations of the Nx enforce-module-boundaries rule.

Related Issue(s)

Fixes #

nx-cloud · 2022-02-08T12:49:52Z

☁️ Nx Cloud Report

CI ran the following commands for commit 63fc1b4. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

✅ Successfully ran 7 targets

Sent with 💌 from NxCloud.

vercel · 2022-02-08T12:49:55Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/nrwl/nx-dev/7toHFRSqF41vx78BFjEHnN4yfpxR
✅ Preview: Canceled

[Deployment for 63fc1b4 canceled]

juristr · 2022-02-28T13:58:35Z

@meeroslav @JamesHenry can some of you please have a look at this? Whether this fits into the current Nx linter setup and anything I might have missed. Thanks a lot :)

meeroslav

Thanks for this extensive addition. I left some notes.

meeroslav · 2022-02-28T14:09:53Z

packages/eslint-plugin-nx/src/rules/enforce-module-boundaries.ts

@@ -228,6 +238,95 @@ export default createESLintRule<Options, MessageIds>({
          data: {
            npmScope,
          },
+          fix(fixer) {
+            const targetImportProject = getTargetProjectBasedOnRelativeImport(


We are running the isRelative twice - as part of isRelativeImportIntoAnotherProject and again at getTargetProjectBasedOnRelativeImport.

I think we should split the condition on line 225 to check for relative and absolute import separately. That way we don't have to check for targetProjectImport twice.

Slightly changed the current implementation to avoid duplicate calls. I guess the only concern there would be mostly around findTargetProject that gets called as part of getTargetProjectBasedOnRelativeImport

meeroslav · 2022-02-28T14:09:58Z

packages/workspace/src/utils/runtime-lint-utils.ts

-  sourceProject: ProjectGraphProjectNode
-): boolean {
+  sourceFilePath: string
+) {
  if (!isRelative(imp)) return false;


The boolean return doesn't seem right based on the function's name. It should be null or undefined. Please add also return type to the function.

meeroslav · 2022-02-28T14:17:17Z

packages/eslint-plugin-nx/src/rules/enforce-module-boundaries.ts

+                  (node.source.parent as any).specifiers || []
+                ).map((specifier) => specifier.imported.name);
+
+                // ignore non-named imports for now, like "import '../../some-other-lib/src'"


This should have been picked up above where we check if it's an import into another package.
Additionally, the example shows relative import, while the condition on line 354 explicitly checks against those.

What is the purpose of this early bail? Can we document this in the E2E test or unit test?

This should have been picked up above where we check if it's an import into another package.

Where exactly?

Additionally, the example shows relative import, while the condition on line 354 explicitly checks against those.

Yeah this is a copy & paste comment error 😉. I'll adjust it. Basically this just documents the fact that imports like import '@myorg/somelib' are not touched by the fixer implementation for now.

What is the purpose of this early bail? Can we document this in the E2E test or unit test?

😅 it's mostly because I didn't implement this case. Might be scope of a future improvement of this fix. So in the case of a relative import across library boundaries, the fix would be pretty easy, like

import from '../../../someotherlib/src' => import from '@myorg/someotherlib'

In the case of a local import to the same lib, the resolution would probably not be what we want:

import from '@myorg/samelib' => import from '../' or whatever the current file location is relative to index.ts of the same lib

I feel this would probably not be what the dev intended though, but rather you'd want to import directly from the relative file in the same lib. Which would be hard to find without a named import.

In general I think these are edge cases though, but we might totally add it (I'd prefer in a future PR)

Please leave a TODO as a reminder

meeroslav · 2022-02-28T14:25:31Z

packages/eslint-plugin-nx/src/rules/enforce-module-boundaries.ts

+                      });
+                    } else {
+                      // we cannot remap, so leave it as is
+                      importsToRemap.push({


What would be the scenario when this happens? A missing path in tsconfig?

Can we have this scenario in tests?

Well this is basically the exit path for all the scenarios which I've either not considered, or which result due to some weird path mapping or something. In those cases the fixer should behave as if it isn't there, therefore defaulting back to not doing anything and just having the error surface to the dev

meeroslav · 2022-02-28T14:27:05Z

packages/eslint-plugin-nx/src/rules/enforce-module-boundaries.ts

+                }
+
+                // group imports from the same relative path
+                const importsToRemapGrouped = importsToRemap.reduce(


Can the two importToRemap reducers be extracted to a runtime linter util function?

ofc, fixing it

meeroslav · 2022-03-08T10:38:10Z

packages/workspace/src/tslint/nxEnforceModuleBoundariesRule.ts

@@ -134,16 +134,18 @@ class EnforceModuleBoundariesWalker extends Lint.RuleWalker {
      this.projectPath
    );
    const sourceProject = findSourceProject(this.projectGraph, filePath);
+    const targetImportProject = getTargetProjectBasedOnRelativeImport(


If targetImportProject is defined, we can skip the findProjectUsingImport on line 159.

It should be something like:

const targetProject = targetImportProject || findProjectUsingImport( this.projectGraph, this.targetProjectLocator, filePath, imp, this.npmScope );

meeroslav · 2022-03-08T11:35:59Z

packages/eslint-plugin-nx/src/utils/ast-utils.ts

+
+function tryReadBaseJson() {
+  try {
+    return JSON.parse(


Devkit has readJson that wraps this segment

…braries and noSelfCircularDependencies

github-actions · 2023-03-19T04:05:11Z

This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request.

vercel bot temporarily deployed to Preview February 8, 2022 12:49 Inactive

juristr force-pushed the no-self-circular-dep-rule-fix branch from bc55bf1 to 5c031db Compare February 8, 2022 13:27

vercel bot temporarily deployed to Preview February 8, 2022 13:27 Inactive

juristr force-pushed the no-self-circular-dep-rule-fix branch from 5c031db to 87c387b Compare February 9, 2022 14:37

vercel bot temporarily deployed to Preview February 9, 2022 14:37 Inactive

juristr force-pushed the no-self-circular-dep-rule-fix branch from 87c387b to 22521f6 Compare February 9, 2022 20:25

vercel bot temporarily deployed to Preview February 9, 2022 20:25 Inactive

juristr force-pushed the no-self-circular-dep-rule-fix branch from 22521f6 to 043b333 Compare February 9, 2022 20:28

vercel bot temporarily deployed to Preview February 9, 2022 20:28 Inactive

vercel bot temporarily deployed to Preview February 9, 2022 21:16 Inactive

juristr force-pushed the no-self-circular-dep-rule-fix branch from af8732a to 0690d25 Compare February 9, 2022 21:57

vercel bot temporarily deployed to Preview February 9, 2022 21:57 Inactive

juristr force-pushed the no-self-circular-dep-rule-fix branch from 0690d25 to 4d66187 Compare February 10, 2022 20:18

vercel bot temporarily deployed to Preview February 10, 2022 20:18 Inactive

FrozenPandaz assigned juristr Feb 10, 2022

juristr force-pushed the no-self-circular-dep-rule-fix branch from 4d66187 to c6a44c4 Compare February 11, 2022 16:30

vercel bot temporarily deployed to Preview February 11, 2022 16:30 Inactive

juristr force-pushed the no-self-circular-dep-rule-fix branch from c6a44c4 to 27543b3 Compare February 11, 2022 20:21

vercel bot temporarily deployed to Preview February 11, 2022 20:22 Inactive

juristr marked this pull request as ready for review February 11, 2022 20:22

juristr force-pushed the no-self-circular-dep-rule-fix branch from 27543b3 to 814732b Compare February 11, 2022 20:55

vercel bot temporarily deployed to Preview February 11, 2022 20:55 Inactive

juristr force-pushed the no-self-circular-dep-rule-fix branch from 814732b to f0498a5 Compare February 24, 2022 14:37

vercel bot temporarily deployed to Preview February 24, 2022 14:37 Inactive

juristr force-pushed the no-self-circular-dep-rule-fix branch from f0498a5 to 0ff5e39 Compare February 24, 2022 20:52

vercel bot temporarily deployed to Preview February 24, 2022 20:52 Inactive

juristr force-pushed the no-self-circular-dep-rule-fix branch from 0ff5e39 to cd4fd89 Compare February 26, 2022 16:24

vercel bot temporarily deployed to Preview February 26, 2022 16:25 Inactive

juristr requested review from meeroslav and JamesHenry February 28, 2022 13:57

meeroslav requested changes Feb 28, 2022

View reviewed changes

juristr force-pushed the no-self-circular-dep-rule-fix branch from cd4fd89 to ebb34b2 Compare March 2, 2022 17:18

vercel bot temporarily deployed to Preview March 2, 2022 17:18 Inactive

juristr force-pushed the no-self-circular-dep-rule-fix branch from ebb34b2 to f87e990 Compare March 2, 2022 17:53

vercel bot temporarily deployed to Preview March 2, 2022 17:53 Inactive

meeroslav added the scope: linter Issues related to Eslint support in Nx label Mar 3, 2022

juristr force-pushed the no-self-circular-dep-rule-fix branch from f87e990 to 485e1ef Compare March 3, 2022 14:46

vercel bot temporarily deployed to Preview March 3, 2022 14:46 Inactive

juristr force-pushed the no-self-circular-dep-rule-fix branch from 485e1ef to df122e6 Compare March 3, 2022 16:24

vercel bot temporarily deployed to Preview March 3, 2022 16:24 Inactive

juristr requested a review from meeroslav March 3, 2022 16:25

meeroslav self-assigned this Mar 7, 2022

meeroslav reviewed Mar 8, 2022

View reviewed changes

meeroslav force-pushed the no-self-circular-dep-rule-fix branch from df122e6 to eec9fb9 Compare March 8, 2022 12:15

vercel bot temporarily deployed to Preview March 8, 2022 12:15 Inactive

meeroslav approved these changes Mar 8, 2022

View reviewed changes

vercel bot temporarily deployed to Preview March 8, 2022 14:16 Inactive

meeroslav enabled auto-merge (squash) March 8, 2022 14:39

juristr and others added 2 commits March 9, 2022 08:40

feat(linter): automatic fixes for noRelativeOrAbsoluteImportsAcrossLi…

f51855d

…braries and noSelfCircularDependencies

feat(linter): minor PR cleanup

63fc1b4

meeroslav force-pushed the no-self-circular-dep-rule-fix branch from d0d08b7 to 63fc1b4 Compare March 9, 2022 07:40

vercel bot temporarily deployed to Preview March 9, 2022 07:40 Inactive

meeroslav merged commit 45bf0e5 into nrwl:master Mar 9, 2022

github-actions bot locked as resolved and limited conversation to collaborators Mar 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(linter): automatic fixes for noRelativeOrAbsoluteImportsAcrossLibraries and noSelfCircularDependencies #8901

feat(linter): automatic fixes for noRelativeOrAbsoluteImportsAcrossLibraries and noSelfCircularDependencies #8901

juristr commented Feb 8, 2022

nx-cloud bot commented Feb 8, 2022 •

edited

vercel bot commented Feb 8, 2022 •

edited

juristr commented Feb 28, 2022

meeroslav left a comment

meeroslav Feb 28, 2022

juristr Mar 2, 2022

meeroslav Feb 28, 2022

meeroslav Feb 28, 2022

juristr Feb 28, 2022

meeroslav Mar 8, 2022

meeroslav Feb 28, 2022

juristr Feb 28, 2022

meeroslav Feb 28, 2022

juristr Mar 2, 2022

meeroslav Mar 8, 2022

meeroslav Mar 8, 2022

github-actions bot commented Mar 19, 2023

feat(linter): automatic fixes for noRelativeOrAbsoluteImportsAcrossLibraries and noSelfCircularDependencies #8901

feat(linter): automatic fixes for noRelativeOrAbsoluteImportsAcrossLibraries and noSelfCircularDependencies #8901

Conversation

juristr commented Feb 8, 2022

Current Behavior

Expected Behavior

Related Issue(s)

nx-cloud bot commented Feb 8, 2022 • edited

☁️ Nx Cloud Report

vercel bot commented Feb 8, 2022 • edited

juristr commented Feb 28, 2022

meeroslav left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Mar 19, 2023

nx-cloud bot commented Feb 8, 2022 •

edited

vercel bot commented Feb 8, 2022 •

edited