[no-extraneous-dependencies] relative path that refers to file outside package.json should error

[bruce-c-liu]

Hi, I'm not sure if the current behavior is intended or not, so I want to start a discussion.

Simple setup

Directory

src/
├── package.json
├── node_modules/
├── .eslintrc.json
├── tsconfig.json
├── client/
├── server/
│   ├── package.json
│   ├── node_modules/
│   └── index.ts
└── shared/
    └── util.ts

tsconfig

{
  "compilerOptions": {
    "paths": {
      "@/utils/*": ["./utils/*"]
    }
}

eslint

"import/no-extraneous-dependencies": ["error", { "packageDir": [".", "./server"] }],

Example 1 with TS Path Alias: Errors as expected

However, the below uses the typescript path alias, which does error. In this case, the plugin resolves it as an external import. https://github.com/import-js/eslint-plugin-import/blob/main/src/core/importType.js#L100

// src/server/index.ts
import util from '@/shared/util';

Example 2 with relative import: No error (Bug?)

When doing the following, there is no error from the rule. This is because the plugin considers it a parent import. https://github.com/import-js/eslint-plugin-import/blob/main/src/core/importType.js#L97

// src/server/index.ts
import util from '../shared/util';

Discussion

I'm not sure I agree with the discrepancy in the above behavior. It doesn't really make sense to me that both methods resolve to the same file, but the rule treats them differently. I think using a relative path to refer to a file outside the enclosing package.json context should cause no-extraneous-dependencies to error.

Next Steps / Solution (?)

The core of the issue seems to be that the typeTest() function is overloaded. In this case, it actually seems like it's a bug when used for this rule. In Example 2, typeTest returns "parent", causing the rule to fail to report the error. The correct behavior was actually for typeTest to return "external".

(aside: I think the above bug makes isExternalModule() and isExternalModuleMain() bugged as well)

@ljharb Is the above assessment accurate?

[ljharb]

The rule treats them differently because they are written differently in the source code - this is a linter, that should be expected.

[bruce-c-liu]

Sorry, I don't follow. Are you saying the rule is currently operating as intended for relative paths that refer to parent files outside of its package.json context?

The rule treats them differently because they are written differently in the source code

I understand how the two cases are handled differently in code. I created this issue to ask if that is intended or a bug.

[ljharb]

It seems intended to me. There's another rule that you can use to prevent relative paths reaching outside of the project dir.

[bruce-c-liu]

It seems intended to me.

Hmm, I don't see it the same way. I believe both examples should error. From the source code, Example 1 considers it an "external" module. In Example 2, it by all rights should also be considered an "external" module, but it's not.

From the rule's description:

Forbid the import of external modules that are not declared in the package.json's dependencies, devDependencies, optionalDependencies, peerDependencies, or bundledDependencies.

I think that falls perfectly in line with the rule, no?

Put another way, why does a relative path not fall into this rule's domain, but a typescript path alias does? 🤔

[ljharb]

Relative paths are always just local files; anything that's not a relative or absolute path is a "bare specifier", which qualifies to maybe be "external".

[bruce-c-liu]

Yes, but how does that preclude this rule from handling relative paths?

Reiterating the rule description again:

Forbid the import of external modules that are not declared in the package.json's dependencies, devDependencies, optionalDependencies, peerDependencies, or bundledDependencies.

By all definitions, Example 2 is importing an external module that is not declared in the package.json.

[ljharb]

external is determined by the plugin's config, and relative paths can't be external. In other words, it's not the english word "external", it's the specific category.

[bruce-c-liu]

I think I understand a little better now.

relative paths can't be external

Perhaps you're referring to the following definition of "external" in the README?

Resolved modules only from those folders will be considered as "external". By default - ["node_modules"].

By that definition, I see what you mean by relative paths can't be external!

However, by that same definition, my Example 1 using typescript path alias should not error. The path alias resolved to a file that is not in node_modules. Hence, by definition it is not an external dependency. 🤔

[ljharb]

isExternalPath returns false, because the import/external-module-folders isn't set, and node_modules is the default, and it's not inside that folder, as you indicated (

eslint-plugin-import/src/core/importType.js

Line 72 in f72f207

const folders = settings && settings['import/external-module-folders'] || ['node_modules'];

).

From my reading of the code, this means "external" isn't defined as "outside the project".

It sounds like @/shared/util should not be erroring, then, and the bug is that it is? (the opposite of your OP)

[bruce-c-liu]

Haha, yes! Now that I understand the definition of "external" used in this plugin, I believe we've made a complete 180 from my OP.

[bruce-c-liu]

#2334
#2333

Here are some related items I found.

[soryy708]

So to summarize,

Example 1 with TS Path Alias

Uses TypesSript path alias.

// src/server/index.ts
import util from '@/shared/util';

Expected: Should not error

Actual: Errors.
In this case, the plugin resolves it as an external import.

https://github.com/import-js/eslint-plugin-import/blob/main/src/core/importType.js#L100

Did I get that correctly?

[bruce-c-liu]

@soryy708 Yes, your understanding is correct.