-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Comments
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. |
@ljharb Yeah, I'm specifically talking about the typescript additions. |
@ljharb - would there be any appetite for
Currently this errors with |
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. |
@ljharb - sorry, do you mind explaining what the Typescript bug/flaw is? Currently I'm struggling to be able to enforce |
@leepowelldev my understanding is that typescript explicitly forces you to omit extensions, so that it can conceptually point to |
@ljharb Typescript doesn't explicitly force you to omit extensions. It won't let you use
I'm not sure what you mean by this. |
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". |
I'm migrating a large project written in TypeScript to output native ESM modules for Node. This requires all the 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 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: "./"
}
}
}
}; |
Would a compromise maybe be to introduce a |
There is also this issue in |
I think the proper place to do that is the TS resolver. It can, for example, resolve otherwise-missing |
Looking at the src for I have to disagree that supporting this feature should be done at the resolver level. |
That suggests to me that the extensions rule needs refactoring, if it’s hardcoding node resolution. |
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 |
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? |
We don’t have any typescript-specific rules; this is a JavaScript plugin that works for TS as well. |
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.
This gives the flexibility without directly supporting Typescript. |
This is actually already supported at import-js/eslint-import-resolver-typescript#56.
@ljharb The problem is there is no way to enforce to use |
and overrides that only apply to .ts files, that set the extension to be “always”, presumably would force .ts, and you want .js? |
Yep. |
Thanks to @JounQin I was able to get this working by excluding
With
|
@patrickarlt so if we just don't use |
@leepowelldev It works unless you also enable the My minimal reproduction of this is at https://github.com/patrickarlt/minimal-typescript-eslint-with-extensions. Adding
|
Unfortunately |
@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 It looks like |
|
If it's helpful, here's an example of using 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 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. |
@tilgovi sure. but if you want your source to include extensions, it should include |
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 Extend the resolver plugin API to let it return two paths, an import path and a physical path. Rules like By default the import and physical paths would be the same, but Is this a solution that would make everyone happy? |
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? |
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. |
@tilgovi Maybe you can draft a PR for it. |
What need is not being met by the TypeScript resolver today? |
TS resolver is working as expected, this plugin does handle extensions correctly. |
I think it's perhaps only the "missing file extension" warnings when using |
I totally agree with this, but it could be a BREAKING change. A new setting can be provided. |
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 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 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? |
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. |
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 |
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! |
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 |
@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. |
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 |
This works but this plugin's But recommended rules with |
Could not reproduce on up to date versions. |
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 theplugin: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.The text was updated successfully, but these errors were encountered: