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

New rule: no-relative-packages #966

Merged
merged 1 commit into from Jan 31, 2021

Conversation

panrafal
Copy link
Contributor

@panrafal panrafal commented Nov 2, 2017

no-relative-packages

Use this rule to prevent importing packages through relative paths.

It's useful in Yarn/Lerna workspaces, were it's possible to import a sibling
package using ../package relative path, while direct package is the correct one.

Examples

Given the following folder structure:

my-project
├── packages
│   ├── foo
│   │   ├── index.js
│   │   └── package.json
│   └── bar
│       ├── index.js
│       └── package.json
└── entry.js

And the .eslintrc file:

{
  ...
  "rules": {
    "import/no-relative-packages": "error"
  }
}

The following patterns are considered problems:

/**
 *  in my-project/packages/foo.js
 */

import bar from '../bar'; // Import sibling package using relative path
import entry from '../../entry.js'; // Import from parent package using relative path

/**
 *  in my-project/entry.js
 */

import bar from './packages/bar'; // Import child package using relative path

The following patterns are NOT considered problems:

/**
 *  in my-project/packages/foo.js
 */

import bar from 'bar'; // Import sibling package using package name

/**
 *  in my-project/entry.js
 */

import bar from 'bar'; // Import sibling package using package name

Use this rule to prevent importing packages through relative paths.

It's useful in Yarn/Lerna workspaces, were it's possible to import a sibling package using `../package` relative path, while direct `package` is the correct one.

Co-authored-by: Rafal Lindemann <rl@stamina.pl>
Co-authored-by: Tom Payne <tom@tompayne.dev>
Co-authored-by: Jordan Harband <ljharb@gmail.com>
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

While I love this use case, I don't think this is the proper way to configure it. What if the foo package had a "src" dir, and a file inside it wanted to require('../')?

I think perhaps a better way to configure a rule like this is that the user would provide a list of paths (globs would be supported), and those paths would be considered boundaries - also called "jails".

So, for your example structure:

"rules": {
  "import/package-boundaries": ["error", {
    "boundaries": ["packages/*"]
  }],
}

That would overlap nicely with lerna/yarn workspaces package settings, and could allow for any imports or requires within a boundary, but nothing could escape the boundary from within.

@panrafal
Copy link
Contributor Author

panrafal commented Nov 4, 2017

@ljharb I would say it's an idea for another rule.

Here, it's not forbidden to use ../ from src, because both 'src' and 'src/..' live in the same package. My proposal detects only, when package boundaries are crossed with relative paths.

What it does, it verifies to which packages both modules (importer and importee) belong. If the package is different - an error is raised. So as long as you stay within same package, you can have any relative paths.

On additional note, with this setup it's possible to make a simple autofix.

@ljharb
Copy link
Member

ljharb commented Nov 4, 2017

How does your rule - since it has no configuration - know what a package boundary is?

One use case that some people have is nested package.jsons within the same project; would that conflict with this rule?

@panrafal
Copy link
Contributor Author

panrafal commented Nov 5, 2017

Every package.json file with a name field defines a package for it's subdirectories, until another package.json defines another one. This is how node_modules, or yarn workspaces work.

With this directory structure:

  • node_modules
    • foo
      • package.json ({"name": "foo"})
  • packages
    • bar
      • node_modules
        • baz
          • package.json ({"name": "baz"})
      • package.json ({"name": "bar"})
  • src
  • package.json ({"name": "root"})

/node_modules/foo is a boundary of package "foo"
/packages/bar is a boundary of package "bar"
/packages/bar/node_modules/baz is a boundary of package "baz"
Anything else from the root directory is a "root" package

Importing ../../src from packages/bar is an error, because you're trying to import part of the root package from bar.
Importing ./node_modules/baz from packages/bar is also an error, because you're trying to import part of baz package from bar.

Both are unsafe because:

  • bar package uses parts of other packages, but don't have to define them in it's dependencies (no-extraneous-dependencies wouldn't catch it)
  • baz package may be hoisted by yarn to a top-level node_modules, which invalidates the relative paths

We're using this rule successfully on our yarn monorepo.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Gotcha, thanks for clarifying.

This should be useful for normal single-package repos also, since they could just as easily require out of their own directory.

@gotbahn
Copy link

gotbahn commented Mar 18, 2018

Looks nice, would love to use it. But PR looks abandoned. Any updates on that?

@ljharb
Copy link
Member

ljharb commented Mar 20, 2018

It's not abandoned; it just needs review from other maintainers, after which I'll rebase and merge it.

@omasback
Copy link

I don't really understand everything in this thread but I was wishing there was a rule to limit the depth of relative paths. like ./ would be ok but ../ and ../../ would not. You could configure the allowed depth. I'm just dealing with a project with a lot of dots and slashes when root-relative paths would be much easier to read. Anyways, just my 2c!

@orm109
Copy link

orm109 commented Jun 21, 2018

any update with this PR ?

@niieani
Copy link

niieani commented Mar 21, 2019

Anything we can do to get this merged?

@fernandopasik
Copy link
Contributor

@benmosher could you help with this one? :D

@teoxoy
Copy link

teoxoy commented Jan 8, 2020

Can I do anything to get this merged? - I find this new rule really useful for monorepos

@ljharb ljharb force-pushed the feature/no-relative-packages branch from 2804af1 to b934ed2 Compare June 7, 2020 06:08
@ljharb
Copy link
Member

ljharb commented Jun 7, 2020

@panrafal I've rebased this, but there seems to be a test failing.

@coveralls
Copy link

Coverage Status

Coverage decreased (-4.9%) to 92.784% when pulling b934ed2 on panrafal:feature/no-relative-packages into 0b81052 on benmosher:master.

5 similar comments
@coveralls
Copy link

Coverage Status

Coverage decreased (-4.9%) to 92.784% when pulling b934ed2 on panrafal:feature/no-relative-packages into 0b81052 on benmosher:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-4.9%) to 92.784% when pulling b934ed2 on panrafal:feature/no-relative-packages into 0b81052 on benmosher:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-4.9%) to 92.784% when pulling b934ed2 on panrafal:feature/no-relative-packages into 0b81052 on benmosher:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-4.9%) to 92.784% when pulling b934ed2 on panrafal:feature/no-relative-packages into 0b81052 on benmosher:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-4.9%) to 92.784% when pulling b934ed2 on panrafal:feature/no-relative-packages into 0b81052 on benmosher:master.

@tapayne88
Copy link
Contributor

@ljharb I've fixed the tests and opened another PR over here #1860

@ljharb ljharb force-pushed the feature/no-relative-packages branch from b934ed2 to 6f5c52c Compare January 31, 2021 17:05
@ljharb ljharb merged commit 6f5c52c into import-js:master Jan 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

10 participants