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

when using typescript recommended config it should allow for imports of js equivialent #2111

Closed
lifeiscontent opened this issue Jun 1, 2021 · 91 comments

Comments

@lifeiscontent
Copy link

lifeiscontent commented Jun 1, 2021

based off the conversation here: microsoft/TypeScript#16577

if a .ts or .tsx file imports a .js file it will resolve .ts .tsx or .js but the plugin:import/typescript doesn't seem to have support for this functionality.

the js extension is used to reference files that WILL be compiled to, so they technically don't exist, but this is how the TS team is allowing people to write ESM compatible modules in TypeScript.

It would be nice to have this eslint plugin to work with this out of the box so I can enforce that all of my modules import .js files to make sure I'm not missing anything when exporting my package to the web.

@ljharb
Copy link
Member

ljharb commented Jun 1, 2021

The "recommended" config of course doesn't support any typescript functionality - the default is (should always be) JavaScript.

https://github.com/benmosher/eslint-plugin-import#typescript may be helpful.

@lifeiscontent
Copy link
Author

@ljharb Yeah, I'm specifically talking about the typescript additions.

@leepowelldev
Copy link

@leepowelldev
Copy link

@ljharb - would there be any appetite for import/extensions rule to support a typescript option which would allow .ts and .tsx to be equivalent to a .js extension which the Typescript compiler allows us to use as mentioned by @lifeiscontent

// foo.ts
export const foo = 'foo';

// bar.ts
import { foo } from './foo.js';

Currently this errors with Missing file extension "ts" for "./foo.js". Maybe we could add an additional option? I'm happy to take a stab at it if it's something that was welcomed?

@ljharb
Copy link
Member

ljharb commented Sep 1, 2021

I don’t think that would really make sense. It would be modifying a general rule with typescript-specific logic to paper over a typescript bug/flaw - one that once they fix, we’d be stuck supporting the workaround for forever.

@leepowelldev
Copy link

@ljharb - sorry, do you mind explaining what the Typescript bug/flaw is? Currently I'm struggling to be able to enforce .js extensions in a project using Typescript using this rule (which will go on to be published as a ESM package)

@ljharb
Copy link
Member

ljharb commented Sep 1, 2021

@leepowelldev my understanding is that typescript explicitly forces you to omit extensions, so that it can conceptually point to .ts at dev time but .js at runtime. TS and native ESM are effectively not yet compatible, and can't be used together.

@leepowelldev
Copy link

@ljharb Typescript doesn't explicitly force you to omit extensions. It won't let you use .ts ot .tsx as an extension, but .js is perfectly valid (microsoft/TypeScript#16577). This allows me to create valid ESM output from a Typescript source.

TS and native ESM are effectively not yet compatible, and can't be used together.

I'm not sure what you mean by this.

@ljharb
Copy link
Member

ljharb commented Sep 1, 2021

I mean that if your goal is a native ESM package (something i'd strongly discourage for many reasons, unrelated to this), then authoring in TypeScript is not yet an ideal way to get there, because of node's unfortunate "required extensions for ESM" choice.

What I'd suggest is to only use tsc as a typechecker, to use babel as your only transpiler, and then to use a babel plugin to transform the proper omitted-extensions source to whatever output you like, which can include "native ESM with extensions".

@patrickarlt
Copy link

I'm migrating a large project written in TypeScript to output native ESM modules for Node. This requires all the import statements in TypeScript to end in .js so they get output with the .js extensions and I would like to validate that behavior.

I don't think TypeScript is going to budge on their treatment of extensions, see microsoft/TypeScript#16577 (comment), neither will Node JS back off of required extensions for ESM so it is perfectly valid to write this:

// foo.ts
export const foo = 'foo';

// bar.ts
import { foo } from './foo.js';

Currently this config produces Missing file extension "ts" for "./foo.js" which is illogical since TypeScript doesn't actually support .ts on file paths microsoft/TypeScript#37582.

module.exports = {
  root: true,
  parser: "@typescript-eslint/parser",
  parserOptions: {
    ecmaVersion: 2018,
    sourceType: "module"
  },
  plugins: ["@typescript-eslint", "import"],
  extends: ["plugin:import/recommended", "plugin:import/typescript"],
  rules: {
    "import/extensions": ["error", "ignorePackages"]
  },
  settings: {
    "import/extensions": [".js", ".jsx"],
    "import/parsers": {
      "@typescript-eslint/parser": [".ts", ".tsx"]
    },
    "import/resolver": {
      typescript: {
        project: "./"
      }
    }
  }
};

@patrickarlt
Copy link

Would a compromise maybe be to introduce a validExentions option so we could say that .js is a valid extension in .ts files?

@patrickarlt
Copy link

There is also this issue ineslint-import-resolver-typescript import-js/eslint-import-resolver-typescript#80 that mentions this exact use case.

@ljharb
Copy link
Member

ljharb commented Sep 9, 2021

I think the proper place to do that is the TS resolver. It can, for example, resolve otherwise-missing .js files to their present .ts equivalent.

@leepowelldev
Copy link

Looking at the src for extensions rule I believe this plugin would still error even if anything was done in the resolver.

I have to disagree that supporting this feature should be done at the resolver level.

@ljharb
Copy link
Member

ljharb commented Sep 9, 2021

That suggests to me that the extensions rule needs refactoring, if it’s hardcoding node resolution.

@leepowelldev
Copy link

I'm not sure about that ... I think it resolves just fine (would need to check), but then simply check that the file extensions match. Obviously it fails as expected when it tries to match .js with .ts or .tsx. We would need additional checks to allow this to be valid.

@patrickarlt
Copy link

I just opened import-js/eslint-import-resolver-typescript#82 so we can see what might be able to be done in the resolver. Although I do agree with @leepowelldev that it makes more sense to handle this behavior in the rules not the resolver.

This behavior is really specific to TypeScript though so would this be best as a separate rule just for TypeScript so it doesn't conflict?

@ljharb
Copy link
Member

ljharb commented Sep 9, 2021

We don’t have any typescript-specific rules; this is a JavaScript plugin that works for TS as well.

@leepowelldev
Copy link

Sure, and I don't think this has to be typescript specific if we can expose a config option to support alternative relationships between extensions.

"import/extensions": [
  <severity>,
  "never" | "always" | "ignorePackages",
  {
    ignorePackages: true | false,
    pattern: {
      <extension>: "never" | "always" | "ignorePackages"
    },
    mapExtensions: {
      "js": "tsx?"
    }
  }
]

This gives the flexibility without directly supporting Typescript.

@JounQin
Copy link
Collaborator

JounQin commented Sep 9, 2021

This is actually already supported at import-js/eslint-import-resolver-typescript#56.

Just remove "import/extensions" this setting.

This resolver resolves the .ts file correctly, but it's not allowed in your configuration.

@ljharb The problem is there is no way to enforce to use .js extension currently for .ts files.

@ljharb
Copy link
Member

ljharb commented Sep 9, 2021

and overrides that only apply to .ts files, that set the extension to be “always”, presumably would force .ts, and you want .js?

@JounQin
Copy link
Collaborator

JounQin commented Sep 9, 2021

and overrides that only apply to .ts files, that set the extension to be “always”, presumably would force .ts, and you want .js?

Yep.

@patrickarlt
Copy link

Thanks to @JounQin I was able to get this working by excluding eslint-import-resolver-typescript which seems a but counter initiative but it is working. If anyone wants to take a look at the minimal reproduction I made I can push it up to GitHub.

// src/bar.ts - should pass
import { foo } from "./foo.js";

// src/baz.ts - should fail
import { foo } from "./foo";

// src/foo.ts
export const foo = "foo";

With eslint-import-resolver-typescript - both bar.ts and baz.ts fail

2021-09-09_12-22-27

{
  "root": true,
  "parser": "@typescript-eslint/parser",
  "parserOptions": {
    "ecmaVersion": 12,
    "sourceType": "module"
  },
  "plugins": [
    "import"
  ],
  "rules": {
    "import/extensions": ["error", "ignorePackages"]
  },
  "settings": {
    "import/parsers": {
      "@typescript-eslint/parser": [
        ".ts",
        ".tsx"
      ]
    },
    "import/resolver": {
      "typescript": {
        "project": "./tsconfig.json"
      }
    }
  }
}

Without eslint-import-resolver-typescript - works as intended

2021-09-09_12-22-14

{
  "root": true,
  "parser": "@typescript-eslint/parser",
  "parserOptions": {
    "ecmaVersion": 12,
    "sourceType": "module"
  },
  "plugins": [
    "import"
  ],
  "rules": {
    "import/extensions": ["error", "ignorePackages"]
  },
  "settings": {
    "import/parsers": {
      "@typescript-eslint/parser": [
        ".ts",
        ".tsx"
      ]
    }
  }
}

@leepowelldev
Copy link

@patrickarlt so if we just don't use eslint-import-resolver-typescript then this works?

@patrickarlt
Copy link

patrickarlt commented Sep 9, 2021

@leepowelldev It works unless you also enable the import/no-unresolved rule. In my quest to make a VERY minimal reproduction I didn't extend plugin:import/recommended and plugin:import/typescript. So if you don't use eslint-import-resolver-typescript and keep import/no-unresolved disabled it works which I don't really think is intended. I think this is what was discussed in #2111 (comment). The resolver works but the import/extensions rule doesn't think the extension is correct.

My minimal reproduction of this is at https://github.com/patrickarlt/minimal-typescript-eslint-with-extensions.

Adding import/no-unresolved without eslint-import-resolver-typescript breaks resolving (predictable)

2021-09-09_12-50-07

{
  "root": true,
  "parser": "@typescript-eslint/parser",
  "parserOptions": {
    "ecmaVersion": 12,
    "sourceType": "module"
  },
  "plugins": [
    "import"
  ],
  "extends": [
    "plugin:import/recommended",
    "plugin:import/typescript"
  ],
  "rules": {
    "import/extensions": ["error", "ignorePackages"]
  },
  "settings": {
    "import/parsers": {
      "@typescript-eslint/parser": [
        ".ts",
        ".tsx"
      ]
    }
  }
}

Adding import/no-unresolved + eslint-import-resolver-typescript breaks (incorrect ./foo.js is valid TypeScript)

2021-09-09_12-53-07

{
  "root": true,
  "parser": "@typescript-eslint/parser",
  "parserOptions": {
    "ecmaVersion": 12,
    "sourceType": "module"
  },
  "plugins": [
    "import"
  ],
  "extends": [
    "plugin:import/recommended",
    "plugin:import/typescript"
  ],
  "rules": {
    "import/extensions": ["error", "ignorePackages"]
  },
  "settings": {
    "import/parsers": {
      "@typescript-eslint/parser": [
        ".ts",
        ".tsx"
      ]
    },
    "import/resolver": {
      "typescript": {
      }
    }
  }
}

@ljharb
Copy link
Member

ljharb commented Sep 9, 2021

Unfortunately no-unresolved is necessary to detect missing untyped packages, but perhaps adding an option to no-unresolved might be the solution here?

@patrickarlt
Copy link

patrickarlt commented Sep 9, 2021

@ljharb I think the behavior everyone is asking for is this:

Using this config:

{
  "root": true,
  "parser": "@typescript-eslint/parser",
  "parserOptions": {
    "ecmaVersion": 12,
    "sourceType": "module"
  },
  "plugins": [
    "import"
  ],
  "extends": [
    "plugin:import/recommended",
    "plugin:import/typescript"
  ],
  "rules": {
    "import/extensions": ["error", "ignorePackages"]
  },
  "settings": {
    "import/parsers": {
      "@typescript-eslint/parser": [
        ".ts",
        ".tsx"
      ]
    },
    "import/resolver": {
      "typescript": {
      }
    }
  }
}

And these files

// src/bar.ts - should pass
import { foo } from "./foo.js";

// src/baz.ts - should fail
import { foo } from "./foo";

// src/foo.ts
export const foo = "foo";

The expected output should be:

Currently it is:

This is because import { foo } from "./foo.js"; is valid TypeScript that compiles to import { foo } from "./foo.js"; If you want the extensions to output from the TypeScript compiler you must add them in the source.

It looks like eslint-import-resolver-typescript resolves to the correct file but imports/extensions rejects that you can import .js from a .ts file.

@ljharb
Copy link
Member

ljharb commented Feb 10, 2022

  1. In general, I suggest using babel to transpile TypeScript; it does a better job than tsc does (tsc works great for typechecking TypeScript, ofc)
  2. TS refuses to rewrite specifiers
  3. rewriting specifiers with a babel plugin is trivial
  4. source code should not include extensions (node's decision to require it is an ergonomics loss, and doesn't need to damage your codebase if you're using a build process)
  5. So, if you want to output node-native ESM for some reason, my suggestion is to use a babel plugin that adds in the extensions for you, and since linting only applies to the actual source code, you can omit extensions there.

@tilgovi
Copy link

tilgovi commented Feb 10, 2022

If it's helpful, here's an example of using babel-plugin-module-resolver to do the rewriting: https://github.com/apache/incubator-annotator/blob/main/babel.config.js#L59

The heuristic there is that all relative paths from a .ts file are expected to resolve to a .ts file, but you can use your own heuristic or check for the existence of the target paths.

You can use the related import resolver to have ESLint and this plugin resolve using these rewrites: https://github.com/apache/incubator-annotator/blob/main/.eslintrc.js#L62

I think @ljharb has solid recommendations here. Until this all settles down more, it's easier to work around the rough edges with Babel in the toolchain and let tsc just do typechecking.

I don't agree with point 4, but I won't offer more on my own opinion there. I think this is the kind of opinion that @manovotny was politely suggesting isn't terribly helpful.

@ljharb
Copy link
Member

ljharb commented Feb 10, 2022

@tilgovi sure. but if you want your source to include extensions, it should include .ts, and your build should transform that to .mjs.

@andersk
Copy link
Contributor

andersk commented Mar 9, 2022

It’s reasonable that you want to avoid adding TypeScript-specific logic to this JavaScript-centered plugin. What do you think about making the issue become eslint-import-resolver-typescript’s problem, as follows?

Extend the resolver plugin API to let it return two paths, an import path and a physical path. Rules like extensions that examine the filename itself would work against the import path, while rules like no-unresolved that examine the file’s contents would read it through the physical path.

By default the import and physical paths would be the same, but eslint-import-resolver-typescript could be modified to return foo.js for the import path and foo.ts for the physical path, to match tsc’s expectations. (This would be controlled by an eslint-import-resolver-typescript option of course, so that it could still be used in Babel-like configurations.)

Is this a solution that would make everyone happy?

@ljharb
Copy link
Member

ljharb commented Mar 9, 2022

That definitely seems like an appropriate extension to the API, and would probably solve a lot of issues in TS where the physical path is a d.ts file but the import path is not.

How do you propose extending that in a non-breaking way?

@tilgovi
Copy link

tilgovi commented Jul 22, 2022

Is an extension to the API needed? The current resolve interface returns the physical path, and it's called with the import path. Both paths are known, then.

@JounQin
Copy link
Collaborator

JounQin commented Jul 22, 2022

@tilgovi Maybe you can draft a PR for it.

@tilgovi
Copy link

tilgovi commented Jul 22, 2022

What need is not being met by the TypeScript resolver today?

@JounQin
Copy link
Collaborator

JounQin commented Jul 22, 2022

What need is not being met by the TypeScript resolver today?

TS resolver is working as expected, this plugin does handle extensions correctly.

@tilgovi
Copy link

tilgovi commented Jul 22, 2022

I think it's perhaps only the "missing file extension" warnings when using .js to import from the TypeScript build output files. I'll take a look and see what can be done. Maybe import/extensions should be acting on the import path rather than the resolved path.

@JounQin
Copy link
Collaborator

JounQin commented Jul 22, 2022

import path rather than the resolved path.

I totally agree with this, but it could be a BREAKING change. A new setting can be provided.

@tilgovi
Copy link

tilgovi commented Jul 22, 2022

The extensions rule knows both the import path and the resolved path (if it resolves). It needs to use the resolver to support checking whether the resolved path is a package or not.

Currently, when the path resolves, the extensions rule is getting the expected extension from the resolved path. The error happens because the resolved .ts file doesn't end in the expected .js.

The TypeScript team has made it clear that they don't think TypeScript should transform paths during compilation. They are considering making it possible to reference .ts files in the source, though (microsoft/TypeScript#37582). They're not going to transform .ts into .js in this mode, but expect to use it for compilation targets where the build output is not JavaScript and the runtime isn't Node.js such that the .ts files will actually be present at runtime. Things are even weirder when there are bundlers involved, because there's actually no files imported at runtime.

So we need to be careful not to haphazardly expand the role of the resolver. Let's take a step back and ask what the user expects the extensions error to do. Are they trying to prevent a runtime error? Are they trying to prevent a bundler error? A compilation error? Are they just enforcing a stylistic preference?

@tilgovi
Copy link

tilgovi commented Jul 22, 2022

So we need to be careful not to haphazardly expand the role of the resolver.

For example, just because the resolver might be able to tell us what file should resolve to at runtime doesn't mean that's what the user wants their source to say. That may be what TypeScript users want today for a Node.js runtime target, but it might not be what someone else wants today or tomorrow.

@tilgovi
Copy link

tilgovi commented Jul 22, 2022

If we do want to assume that it's the resolver responsibility to error if a source path is incorrect, we could make the change I was suggesting and use the import path's extension should or should not be present given the options of the extensions rule, as I said above.

As @JounQin said, that'd be a breaking change. And as I said, that'd be enshrining a particular role for the resolver that may or may not be quite the same as its original purpose. It would be saying that the resolver's role is not just to help this plugin find files to check, but to say whether or not the import is correct. That's a bit different than a stylistic check, though. That's saying it's the resolvers responsibility to hard error when the user writes .ts and should have written .js. Instead of resolving the .ts file, it should say no, ./foo.ts does not resolve.

@JounQin
Copy link
Collaborator

JounQin commented Jul 22, 2022

@tilgovi

The new behavior can be enabled under a new setting flag, then there will be no breaking change.

If you can contribute to draft a PR, that would be great!

@tilgovi
Copy link

tilgovi commented Jul 22, 2022

I'll sleep on it first. I'm not sure how to name the flag or communicate what it does yet. Any suggestions are welcome.

@JounQin
Copy link
Collaborator

JounQin commented Jul 22, 2022

I'll sleep on it first. I'm not sure how to name the flag or communicate what it does yet. Any suggestions are welcome.

Good night. How about preferImportExtension?

@justingrant
Copy link

@JounQin @tilgovi Was there a conclusion from the discussion above about a "desired file extension" for TS relative imports?

BTW, another thing that could be helpful about this option would be if this plugin were extended in the future to support auto-fix of this problem by adding missing extensions... although for non-TS files I'm not sure how valuable that would be.

@jordaaash
Copy link

FWIW, I wrote an eslint plugin that requires js extensions, and also fixes them: https://github.com/solana-labs/eslint-plugin-require-extensions

@tpluscode
Copy link

FWIW, I wrote an eslint plugin that requires js extensions, and also fixes them: https://github.com/solana-labs/eslint-plugin-require-extensions

Finally, the savior has arrived. Thank you for that

@leppaott
Copy link

leppaott commented Aug 21, 2023

FWIW, I wrote an eslint plugin that requires js extensions, and also fixes them: https://github.com/solana-labs/eslint-plugin-require-extensions

This works but this plugin's ignorePackages would be needed to be usable. A shame this hasn't been resolved yet.

But recommended rules with 'import/no-unresolved': 'off', indeed work see @patrickarlt 's commend.

@soryy708
Copy link
Collaborator

Could not reproduce on up to date versions.

Note: TypeScript allowImportingTsExtensions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests