palantir / tslint

:vertical_traffic_light: An extensible linter for the TypeScript language
http://palantir.github.io/tslint/
Apache License 2.0
5.91k stars 889 forks source link

Configurable no-implicit-dependencies Ignore to Support Absolute Path Imports #3483

Closed bmarcaur closed 6 years ago

bmarcaur commented 7 years ago

Bug Report

TypeScript code being linted

# webpack config
{
  #
  # omitted
  #
  resolve: {
    alias: {
      "react/lib/ReactMount": "react-dom/lib/ReactMount",
      react: __dirname + "/node_modules/react",
      "#": SRC_DIR,
    },
    extensions: [".json", ".js", ".ts", ".tsx", ".less", ".css"],
    modules: [SRC_DIR, "node_modules"],
    mainFields: ["module", "jsnext:main", "browser", "main"],
  },
  #
  # omitted
  #
}

# Some later file
import {CollectorState} from "#/store/collectorState";

with tslint.json configuration:

{
  "extends": [
    "tslint:latest",
    "tslint-config-prettier"
  ],
  "rules": {
    "interface-name": false,
    "member-access": false,
    "object-literal-sort-keys": false,
    "arrow-parens": false,
    "no-empty-interface": false,
    "no-console": false,
    "no-submodule-imports": [true, "#"],
    "max-classes-per-file": [true, 3]
  }
}

Actual behavior

src/services/SearchService.ts
ERROR: 5:43   no-implicit-dependencies  Module '#' is not listed as dependency in package.json

Expected behavior

I use webpack resolve to support absolute path imports and tslint, on two different linters (no-submodule-imports and no-implicit-dependencies), picks this up as a package import and throws an error. My ask is the ability to configure an ignore for certain import prefixes (or packages in this scenario). Similar to:

    "no-submodule-imports": [true, "#"],

I would like to keep the rule enabled as it did help me catch an implicit import but it breaks the above workflow.

ajafff commented 7 years ago

There's just one question: how to add the whitelisted modules to the existing config?

Currently this rule has two config options, so you can write "no-submodule-imports": [true, "dev", "optional"] without whitelisting dev or optional. Simply adding the whitelisted dependencies after that (or in between) makes the configuration rather confusing. In addition there is the possiblity of conflict between config options and actual package names.

I think the best approach would be a config object:

"whitespace": [
  true,
  {
    "dev": true,
    "optional": true,
    "whitelist": ["#", "~"] // property could be named "ignore" as well
]
happycollision commented 6 years ago

An aside: I think @bmarcaur is looking to change the no-implicit-dependencies rule, not the no-submodule-imports rule, or the whitespace rule. You may already know this, but it is unclear to me if we are all on the same page.

My question/suggestion: Is it possible to allow a config object OR two strings following true so that it does not break any current configs? So that either of the following would be acceptable:

"no-implicit-dependencies": [
  true,
  {
    "dev": true,
    "optional": true,
    "allow": ["#", "~", "/app"] // I like "allow" as the exception name, but ditch it if it conflicts with established conventions
  }
]

"no-implicit-dependencies": [ true, "dev", "optional" ]
bmarcaur commented 6 years ago

Yeah sorry my example above uses another tslint rule to show an example of what I would want not that I want to change no-submodule-imports. Sorry that is a bit confusing. Also I imagine that you want to support a heterogeneous API here where it supports both the old and the new format @ajafff to avoid the API break. For example:

"no-implicit-dependencies": [
  true,
  {
    "dev": true,
    "optional": true,
    "allow": ["#", "~", "/app"] // I like "allow" as the exception name, but ditch it if it conflicts with established conventions
  },
  "dev",
  "optional"
]

edit: I started working on this but then I realized that I broke the API. So unfortunately no real progress to show. Ill take a look at this again when i have time over the holidays.

viridia commented 6 years ago

I just ran into this as well - I have no choice but to disable these rules in my tslint.json.

happycollision commented 6 years ago

@viridia: You might consider doing some inline tslint disabling. That way, you'd still get feedback if you accidentally import something that is not in your package file.

That's what I settled on in the meantime. Of course, for my project, I didn't have very many places where I was importing in a way that caused an issue. If you do, it might be a bigger headache.

viridia commented 6 years ago

@happycollision I used absolute imports pervasively throughout the code - based on advice from this medium post: https://spin.atomicobject.com/2017/10/07/absolute-paths-javascript/

Legym commented 6 years ago

I'm also having an issue. This would be so great if we had a fix for this. I ended up disabling the rule in my tslint.json

tdolsen commented 6 years ago

For those interested have I figured out a very hacky workaround, while we are waiting for a real fix.

fchiumeo commented 6 years ago

Any forecast if that will be implemented?

7sDream commented 6 years ago

Need this enhancement too, please consider implement it. :rose:

vladnicula commented 6 years ago

I would also benefit from this.

benjaminbillet commented 6 years ago

I had to disable the no-implicit-dependencies rule because I use absolute imports in my projects. This feature could be very useful.

tbsvttr commented 6 years ago

Need this feature here, too.

formatlos commented 6 years ago

this feature would be very much appreciated

fchiumeo commented 6 years ago

https://github.com/palantir/tslint/releases/tag/5.11.0

v5.11.0 [new-rule-option] Add whitelist for no-implicit-dependencies (#3979)

jeznag commented 6 years ago

Can documentation be added for this? I don't understand how to use it.

If I have a. devDependencies b. paths listed in tsconfig.json, e.g.

    "paths": {
      "@App/*": [
        "src/*"
      ],
      "@Shared/*": [
        "src/Shared/*"
      ]
    },

How would I use this?

I tried:

    "no-implicit-dependencies": [true, "dev", ["@App", "@Shared"]],

But I get this error:

/Users/jeremynagel/dev/energylink-project/react-components/data-exporter2/src/DataJunction/DataJunctionTable.tsx
(22,8): Module '@Shared/BootstrapTableFormatters' is not listed as dependency in package.json
ilearnio commented 6 years ago

Got it working. Here is what I did:

  1. Upgrade to tslint@^5.11.0

  2. In tslint.json add an array of path's you want to whitelist to no-implicit-dependencies. In my case I only need to whitelist ~, so it looks like:

"no-implicit-dependencies": [true, ["~"]]
  1. In tsconfig.json I have the following paths
    "paths": {
    "~/*": ["./*"]
    },
jeznag commented 6 years ago

What about dev dependencies @ilearnio ?

ilearnio commented 6 years ago

@jeznag Hm, not sure if I'll ever need them in my app's code. So far all works good

chris5287 commented 6 years ago

@jeznag FYI if your paths start with @ then you need to specify the whole name (not just the package name) due to the way the package name is calculated (it seems to not split the name of if it’s starts with an @ symbol): see https://github.com/palantir/tslint/blob/9d7db479596c50bbf89fa87d498ab927763a6eec/src/rules/noImplicitDependenciesRule.ts#L113

arsnl commented 6 years ago

Damn! That's a good catch @chris5287 ! I was using the @ to define my local path. I guess i'll start using the ~ from now.

My tslint.json

{
  "defaultSeverity": "error",
  "extends": ["tslint:latest", "tslint-config-prettier"],
  "rules": {
    "no-submodule-imports": [true, "~root", "~src"],
    "no-implicit-dependencies": [true, ["~root", "~src"]]
  },
  "linterOptions": {
    "exclude": ["node_modules/**/*", "dist/**/*", "test/**/*"]
  }
}

My tsconfig.json

{
  "compilerOptions": {
    "strict": true,
    "module": "ESNext",
    "moduleResolution": "node",
    "target": "ESNext",
    "allowSyntheticDefaultImports": true,
    "outDir": "dist",
    "sourceMap": true,
    "baseUrl": ".",
    "paths": {
      "~root/*": ["./*"],
      "~src/*": ["src/*"]
    }
  }
}

So I can now import local modules without tslint nor ts error.

Eg:

import requestIdMiddleware from '~src/middlewares/request-id';
didaktio commented 4 years ago

Don't forget the wildcard (*) character! Easily missed but critical.

Incorrect:

"paths": {
      "path1/": ["dir/"]
    }

Correct:

"paths": {
      "path1/*": ["dir/*"]
    }
JoshuaKGoldberg commented 4 years ago

πŸ€– Beep boop! πŸ‘‰ TSLint is deprecated πŸ‘ˆ and you should switch to typescript-eslint! πŸ€–

πŸ”’ This issue is being locked to prevent further unnecessary discussions. Thank you! πŸ‘‹