microsoft/tslint-microsoft-contrib

Improvement for no-relative-imports rule: allow parent imports (like we have allow-siblings)

sharikovvladislav opened this issue · 5 comments

Feature request

Is your feature request related to a problem? Please describe.
tslint has rule no-relative-imports out-of-the-box. This rule has allow-siblings option. So we can write paths in two ways:

  • no relative paths at all without option, but using this rule
  • ./* relative paths are allowed with option

In our team we have an arrangement, which allows us to use ../* paths. Only 1 step above, more (like ../../*) is restricted.

This rule + option is not flexible enough for this.

Describe the solution you'd like
I suggest to add one more option, which will specify amount of steps we can go above. Or I think it would be better, to add allow-parent option. It is more strict and applies better to general idea of the rule.

Describe alternatives you've considered
I don't have any ideas of alternatives :) I can just write custom rule for our project. But I think it is some feature that can be used not only by our team.

I can create PR if you don't mind adding this to the project.

Honestly speaking while I understand why would you like mentioned options and behavior, I don't think that this is good fit for existing rule and would be better served as independent more configurable rule.

While allowing ../* contradicts rule name in the same way as ./* later is more common pattern which enables code splitting and rarely hurts code movements.
In other benefits of separate rule - allow-parent behavior might be restricted only to few file or import path patterns with configuration that doesn't exists in current rule.

As workaround for you can use import-blacklist core rule and passing pattern as shown below

{
    "rules": {
        "import-blacklist": [true, ["/\\.\\./"]]
    }
}
import tst from 'test';
import tst from './test';

import tst from '../test';
import tst from '../../test'; // Error
import tst from 'aa/../test'; // Error

import tst = require('aa/bb/test');
import tst = require('../../test'); // Error
import tst = require('aa/../test'); // Error

const tst = require('aa/bb/test');
const tst = require('../../test'); // Error
const tst = require('aa/../test'); // Error

Not the best error message This import is blacklisted by /\/\.\.\//, but can be negotiated to the team

If by some chance you use windows path style - than RegExp in config will be a bit cryptic "import-blacklist": [true, "", ["[/\\\\\\\\]\\.\\.[/\\\\\\\\]"]]

more configurable rule.

in what ways it should be more configurable? depth?

In other benefits of separate rule - allow-parent behavior might be restricted only to few file or import path patterns with configuration that doesn't exists in current rule.

I don't understand this point. Could you explain more verbose, please?

which enables code splitting and rarely hurts code movements.

Does ../* have problems with it? Looks like tools fixing everything allright. Or I don't understand the point again :P

With allow-parent option current rule matches behavior of import/no-relative-parent-imports ESLint rule and I doubt that this rule should move further.

Does arrangement in your team allow ../* imports everywhere or under some conditions?

In my opinion ../* are more prone to bring errors during move (not all tools will update paths correctly) or especially copying and shouldn't be allowed all across the project. Therefore some rules should be enforced to handle path with references to parent folders.

More configuration may be (and will be more project specific):

  • depth
  • condition for module specifier (e.g. parent references are allowed only like from '../../Theme' or from '../style.css', but would be disallowed in from '../connectors')
  • , condition for file location (only files located in sub-component folder can have parent imports, but in config folder they are disallowed)

This kind of configuration is out of scope for no-relative-imports rule.

I will leave this issue open so other maintainers can weight in on your proposal, however I'm not sure that no-relative-imports rule should support any parent references given available workaround with import-blacklist rule form TSLint.

given available workaround with import-blacklist

Its ok. Feel free to close the issue, If you don't think this is applicable for tslint in general.

Also, I can create my custom rule based on any tslint rule and use it. We don't have this problem now. Thank you for import-blacklist