no-relative-parent-imports reports internal packages as parent

[JacobLey]

I recently started working on a rush monorepo, that basically has the setup:

rush.json
/lib
/lib/src/index.js -- export const abc = 123
/apps
/apps/src/index.js -- import { abc } from '@my-package/lib';
/apps/node_modules/@my-package/lib/src/index.js

I have eslint settings configured as:

settings: {
    'import/internal-regex': '^@my-package/',
},

The rule import/no-relative-parent-imports wrongly reports that import { abc } from '@my-package/lib' with error Relative imports from parent directories are not allowed. Please either pass what you're importing through at runtime (dependency injection), move `index.ts` to same directory as `@my-package/lib` or consider making `@my-package/lib` a package.

I suspect this is related to https://github.com/import-js/eslint-plugin-import/issues/1644

Upon some digging, it appears that this library does correctly identify that import as "internal" (therefore rules like import/order work as expected) but then the path is "resolved" to ../node_modules/@my-package/lib/index.js which looks like a parent directory.

This check returns false because it is an internal import, not external absDepthPath resolves to the full node_modules path (e.g. /user/jacob/my-repo/apps/node_modules/@my-package/lib/index.js) then relDepthPath looks like a parent import when converted to a relative path (e.g. ../node_modules/@my-package/lib/index.js)

My suggestion is to simply exit early on both internal and builtin (because why not?) modules instead of trying to resolve the path. Is there a reason it was not implemented that way in the first place? This library has generalized logic for detecting "parent" vs "sibling" logic already?

It is worth noting that I worked with this rule in an npm/lerna workspace and never came across this issue. So I tried investigating those and it appears in those workspaces, node_modules was hoisted to the top-most directory and it appears resolve actually failed to resolve those and it would not throw an error.

So in that sense it seems that Rush is actually providing a better node_modules resolution, and as a consequence these rules which might have allowed internal packages to slip by are now exposing a logical flaw...

Happy to open the PR to allow internal + builtin modules to exit early, but I suspect I am missing a piece of the logic, so opening this issue for discussion!

[ljharb]

While a monorepo blindly hoisting things to the top level is always going to be problematic, I agree that anything that's inside a parent's node_modules directory should not be warned on by this rule.

That a module is marked "internal" or "builtin" doesn't seem relevant.

[tmtron]

@ljharb I also think that internal modules should be ignored.

But maybe it's just a misunderstanding, as I am not sure what "internal" means in this context. So I'll try to explain what I think that the terms mean to me.

I have created a minimal nx monorepo on GitHub to illustrate the issue: GitHub lint-test.

• external: listed in package.json and installed in the node_modules folder, e.g.
  • import from 'lodash';
  • import from '@angular/common';
• internal: in a monorepo, we can have multiple packages/libs
  • these libs are checked-into the VCS and the nrwl/nx makes sure to build them in the right order, etc.
  • these imports look the same as external ones, but they are specified in the path mappings of tsconfig.json
    • "@lint-test/lib-a": ["libs/lib-a/src/index.ts"]
    • "@lint-test/lib-b": ["libs/lib-b/src/index.ts"]
    • these can be imported like this: import { libA } from '@lint-test/lib-a'; ref
• builtin: e.g. import from 'fs' (i.e. node:fs)
  • see: https://github.com/import-js/eslint-plugin-import#importcore-modules

As explained in the readme, when lib-b uses import { libA } from '@lint-test/lib-a', I am sure, that this should not be an error. But currently no-relative-parent-imports flags them as error: see import in lib-a child.ts

Am I missing something?

[trim21]

I write a simple rule to fix this, you will need eslint-plugin-rulesdir

this works with eslint-typescript(TSESTree).

// remove this after https://github.com/import-js/eslint-plugin-import/issues/2467 is fixed

module.exports = {
  meta: {
    type: 'problem',
    docs: {
      description: 'forbidden relative parent import',
      category: 'Stylistic Issues',
    },
    schema: [],
  },

  create: (context) => {
    return {
      'Program > ImportDeclaration': (node) => {
        if (node.source.value.startsWith('../')) {
          context.report({
            node,
            message: 'do not use relative parent import, use absolute import ...',
          });
        }
      },
    };
  },
};

auto fix version

// TODO: remove this after https://github.com/import-js/eslint-plugin-import/issues/2467 is fixed

const path = require('node:path');
const posix = require('node:path/posix');

const { ESLintUtils } = require('@typescript-eslint/utils');

const projectRoot = posix.normalize(path.dirname(__dirname)); // change this according to your script file location

const createRule = ESLintUtils.RuleCreator((name) => name);

module.exports = createRule({
  name: 'no-relative-parent-import',
  meta: {
    type: 'problem',
    docs: {
      recommended: 'error',
      suggestion: true,
      requiresTypeChecking: false,
      description: 'forbidden relative parent import',
    },
    fixable: 'code',
    schema: [],
    messages: {
      import: "do not use relative parent import, use '{{ should }}' instead",
    },
  },
  defaultOptions: [],

  create: (context) => {
    const filename = context.getFilename();
    return {
      ImportDeclaration: (node) => {
        if (node.source.value.startsWith('..')) {
          const importPath = path.resolve(path.dirname(filename), node.source.value);
          const should = posix.normalize(
            posix.join('@app', path.relative(projectRoot, importPath).replaceAll('\\', '/')), // replace it with your own base mod name
          );

          context.report({
            node,
            messageId: 'import',
            data: { should },
            fix: (fixer) => {
              return fixer.replaceTextRange(node.source.range, "'" + should + "'");
            },
          });
        }
      },
    };
  },
});