eslint-plugin-import icon indicating copy to clipboard operation
eslint-plugin-import copied to clipboard

import/no-relative-packages fix breaks in monorepo

Open unleashit opened this issue 3 years ago • 6 comments

After updating my eslint config and running it on a yarn workspaces monorepo, I found that a new rule was added: import/no-relative-packages. I like the rule as it prevents someone from relatively importing something from another package.

But in my case, I have some test utils in a folder at the root of the monorepo which is just a folder and not a namespaced package. I'm currently importing them (relatively) only from some Jest tests. I'm not sure how the fix works, but in this case it seems to be simply assigning the name of the root folder for the project as the absolute path base like:

../../../testConfig/utils (import path before)

npm-library/testConfig/utils (after the fix)

The problem is it doesn't work since Node can't resolve 'npm-library'. Perhaps it does work when fixing a relative import from a namespaced package but I haven't tried.

If there's not way to resolve this automatically, maybe it be a good idea to add an exception to the configuration (like a regex/glob pattern to disable for certain files/directories).

unleashit avatar May 14 '22 02:05 unleashit

P.s. for nodejs (without webpack) that rule broke application

AndreiSoroka avatar Jul 17 '22 22:07 AndreiSoroka

If you can provide a repro repo, or a PR with failing test cases, I'd love to reopen this and fix it.

ljharb avatar Aug 29 '22 22:08 ljharb

In the middle of a move so won't have time in the near future unfortunately...

Maybe I wasn't clear though? My problem is the behavior of the eslint "fix". It attempts to convert the relative path, which for me leads to a parent folder above the monorepo package (but still in the monorepo), to an absolute path based on the project/monorepo root folder name. This breaks tools like Jest since without further configuration since they don't know how how to resolve /<some_name>/path/to/test.ts (where some_name is generated by the fix and seems to be the name of the project/monorepo folder).

The tests fail since they can't resolve the absolute paths that the fix makes.

It seems like a solution would be to allow an exclusion regex to the rule in eslintrc. Then at least I could leave the rule on for non-test files. For now, I've had to turn it off.

unleashit avatar Aug 29 '22 23:08 unleashit

hmm, i think i get what you mean. you're saying that because npm-library, which live in the monorepo, is symlinked into node_modules, the rule is autofixing the path to use the bare specifier?

While we could certainly handle symlinks and treat them differently, I'm not sure why you'd want that - if npm-library/testConfig/utils is resolveable then that seems better.

doesn't work since Node can't resolve 'npm-library'.

This seems like a bug. theoretically we'd resolve the same things as node.

I'll reopen it so we don't lose track of it; the sooner we can get a repro the better :-)

ljharb avatar Aug 29 '22 23:08 ljharb

Thanks for the quick reply! npm-library (not the most creative name I admit) is actually the name of the monorepo, so that's the top parent. It's also the name the fix used as the parent folder for the absolute pathname.

So npm-library doesn't live in the monorepo, it is the monorepo. The packages/apps are in a packages directory, and the tests are within the individual packages. Right under the parent (npm-library) and as a sibling to the package directory is a folder that contains the test configs, which I want to link to from the tests.

This could perhaps be a WSL2 issue because it doesn't always handle symlinks correctly. But a lot of people are on WSL2.

If this isn't enough info, it'll be at least a month from now, but at that point I'll have time to post a repro. So feel free to keep open or closed as you think is best 👍

unleashit avatar Aug 29 '22 23:08 unleashit

Thanks, I'll keep it open, this seems like a pretty important bug to fix.

ljharb avatar Aug 29 '22 23:08 ljharb

I think I have the same problem: My unit test are located inside my package directory but outside src directory:

/monorepo
└── packages
    └── package1
        ├── src
        │   └── foo.js
        ├── test
        │   └── foo.test.js        
        └── package.json

I have a relative import in my unit test file:

foo.test.js

import foo from "../src/foo";

And the rule detects an error and incorrectly fixes it

error Relative import from another package is not allowed. Use monorepo/src/src instead of ../src import/no-relative-packages

Is that the same issue as the one described by this issue? 🤔

yvele avatar Dec 29 '22 13:12 yvele

@yvele it might be the same! if you could create a tiny repro repo for that problem, i could get it fixed ASAP :-)

ljharb avatar Dec 29 '22 18:12 ljharb

I have a similar issue with a different case: I have directory with package.json but they're not part of the workspaces so they don't have symlink in the monorepo root node_modules. This rule auto fix the relative imports path to these packages to absolute paths. I would appreciate it if an option like the packageDir option of the no-extraneous-dependencies rule

chbdetta avatar Jan 26 '23 19:01 chbdetta

@chbdetta It has a package.json with more than type/main fields? What's in it, and why isn't it part of the workspaces?

ljharb avatar Jan 26 '23 20:01 ljharb

@ljharb the package.json also contain name and version. The package is a standalone project that doesn't follow the monorepo release cycle so it's not part of the workspaces

chbdetta avatar Jan 26 '23 21:01 chbdetta

then it should probably have its own eslintrc with root:true in it?

ljharb avatar Jan 26 '23 22:01 ljharb

@yvele yes it's the same. Only difference is I was while my tests were within the source directory, I attempting to import test helpers from a folder above the packages root.

@ljharb I totally respect your time and have no issue with the more recent expectation of maintainers for repros, But unfortunately it does also have the side effect of making bug reporting more difficult and time consuming, especially when it's a package that I didn't personally install or need (it was added along with an update to an Eslint rule group in one of my side projects). To be honest, posting this issue itself was an effort to help you and the community, not myself. Please don't take this as a complaint. I hope someone who actually uses this package (I disabled) and has the problem has the time to post a repro. It's possible if you can't reproduce that its environment related. I'm using WSL on Windows.

unleashit avatar Jan 26 '23 22:01 unleashit

@unleashit i definitely appreciate the report, and it's not an "expectation" as much as it's just not something that's possible for me to fix if i can't reproduce it.

There's always patterns that might be unexpected for libraries, but I'd say that when using workspaces, everything should be using them, and that shouldn't have any relationship to release cycles since packages should only be released when needed ("independent versioning" in lerna's parlance), and either way, if it's an isolated project, i'd expect it to have an isolated eslint config too. Importing something from outside the package root, when the package isn't part of workspaces, is also a highly discouraged pattern in monorepos, which is why there's rules to prevent it.

I'd love to fix the bug, but unless someone can reproduce it, I don't know how I possibly could.

ljharb avatar Jan 26 '23 23:01 ljharb

If you can't reproduce it and it isn't in your good opinion to add an exception to the rule so others who don't feel as strongly as you, then we are free not to use. That's the beauty of open source! I do agree and already mentioned that the rule has a lot of value, I just don't happen to share the opinion that in all cases and purposes there should never be an exception. There's no "slippery slope" in my opinion to do what I'm doing in this case. Sure I could for example make a new monorepo package just for these shared test helpers, but this is a older personal project and well... I'm lazy. Adding to that, since this is just for unit testing and it doesn't share the same environment as a production app, it's a risk I'm willing to take. I realize I'm a black sheep and occasionally question some things that are widely considered best practice, but don't worry... AI is coming for all of us humans and our silly illogical ways soon enough!

unleashit avatar Jan 26 '23 23:01 unleashit

Extensive playing with the available AI options demonstrates that it'll be a very, very long time before that happens :-)

In that case I'll close; if someone else experiences the problem in the future and can provide a repro i'd be happy to fix it.

ljharb avatar Jan 26 '23 23:01 ljharb