-
-
Notifications
You must be signed in to change notification settings - Fork 515
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
useImportExtensions should use .js extension for TypeScript imports #2967
Comments
I do not believe we should do that. Most users are using bundlers, which are perfectly fine resolving .ts files and as of Typescript 5.0 it supports them too allowImportingTsExtensions=true with some caveats. In source code having import file extension |
This is only true for frontend. In backend absolute minority uses bundlers.
It is not confusing. It is what TypeScript team explicitly requests users to do. See this comment from https://stackoverflow.com/questions/62619058/appending-js-extension-on-relative-import-statements-during-typescript-compilat by one of TS devs: "TS does not re-write import paths. This something the TS team has been adamant about it. Write the paths that work at runtime, and if needed configure ts to make those paths work. ( in this case no configuration is needed)"
It is not an option in ESM projects. They require you to have an explicit extension. That is the whole reason why this linting rule (and the autofix) are so important in the first place. Node.js refuses to allow extensionless relative imports |
I think OP has a valid argument. Many library authors (me for example) don't rely on bundlers, but only on the transformation offered by TypeScript. If we suggest using In a bundler world that wouldn't matter, suggesting
ESM doesn't work with imports without extensions, and even |
Ideally we would read tsconfig and based on that option we would choose which extension to suggest. How many people are actually using .js extension in ts files? https://github.com/search?q=Language%3Ats+%22import%22+.js&type=code&p=1 even include false positives there are not many results in GitHub search, for regular TS user I would strongly argue it would be confusing behavior. |
@minht11 All of them who use TS with ESM. |
Also good to mention that using I don’t think all bundlers allow |
@arendjr Best solution would be to make the behaviour directly configureable within the rule options. |
@minht11 So what is your preferred path forward, considering that Bun supports pretty much everything: extensionless, .ts and .js. Deno supports .ts and extensionless by default, and .js with --unstable-sloppy-imports flag (which is not ideal, but still makes it possible to run). So .js still seems to be the most compatible option out of everything available. And considering that Deno and Bun work fine with extensionless imports, and don't need the rule in the first place, doesn't it make sense to optimize for Node, which strictly rely on it for backend ESM TypeScript? |
As someone who has been arguing against the TypeScript team's decision to use If you are in a situation where you need to use
In other words, preferring I wouldn't protest if someone wants to introduce a rule option that caters to the |
You are right, and I wholeheartedly agree with you that TS team made the wrong call. But they made it, and we live in the world where this is reality, and ignoring the consequences of this change is breaking code for developers in real projects.
Which basically means you are a backend Node.js developer who uses TypeScript. Absolute minority of backend services use bundlers.
This is not true. As previously mentioned, Bun supports both options, Deno supports both options with a switch, and bundlers support both versions as well, so this would be a significantly less breaking version of the rule than what it is right now. Not to mention that majority of these users don't need the rule in the first place, as you primarily are forced to use extensions in bundler-less Node.js environments, so you are fine to just disable it. That said, it would only be a breaking change only if it is the only option that the rule supports. Why not making it configureable? That would literally cover every single possible case out there.
[citation needed]. Anecdotally, I have never once encountered a server-side code using a bundler, outside of SSR frameworks.
Can you suggest an API that you would be happy with? Should it be some kind of flexible string for extension, or just a simple toggle of |
Personally, I don't remember the last time using a Node.js project that was not bundled, which isn't surprising given how easy it is to setup with tools such as Of course, it's hard to measure how popular the practice is exactly, but searching for bundling for Node.js gives plenty of recommendations for why and how to do it:
If anything, I find your "Absolute minority of backend services use bundlers." to be a statement in need of citation, since you state it with so much certainty.
You assume that because some tools do support these extensions, people will use configurations in which they are also supported. As a Deno user, I explicitly don't use And as I said before, even for those where it would work, it would still be confusing for them. The practice of manual extension rewriting might seem natural to you if you're used to it, but it's absolutely backwards and unintuitive to anyone else.
I would opt for a string extension here, so it also allows people to choose |
The only viable option here is to rely on the configuration already set in |
@mrhyde I don't think it's possible. As per documentation: "This flag is only allowed when --noEmit or --emitDeclarationOnly is enabled, since these import paths would not be resolvable at runtime in JavaScript output files" So it will never be set to true in a config meant for transpiling runtime code. Can we determine whether project is Node, bun or deno somehow, if aim is to prevent explicit configuration? |
@kibertoad Why not? If: |
@mrhyde Oh, that's what you mean. Yeah, that sounds like a great approach. |
@ematipico I'm happy to immediately bump my monthly sponsorship pledge by $10 if this is addressed by the Biome team in any way that unbreaks Node.js :D |
It's unclear to me what are the next steps to close the issue |
@ematipico We are currently looking into possibility of addressing the issue with a PR from our side. Would you be OK with the solution that @arendjr has proposed, namely, being able to provide custom mapping overrides, where we can explicitly configure which extensions should correspond to which extensions, and the rule would check custom mappings prior to using the currently hardcoded one? Relying on |
It seems to be a fair fix :) |
Making this rule configurable is the best thing to do. I saw many valid arguments for |
In the meantime, if someone needs the "fix" for the rule, you can use this package https://github.com/beenotung/fix-esm-import-path I am migrating a big monorepo with minor issues that I am fixing while discovering them. |
@fix1t I recently published a similar package out of frustration. It requires zero configuration and can be used with npx. https://github.com/2bad/tsfix |
@mrhyde Nice! I prefer having the Kudos! |
Reading the last discussions and taking a look at #3274, I think there is something wrong.
Instead of providing a mapping, I could just provide an option that indicates if we should use the actual extension or the js-ified extension (for ts-like extensions).
|
@Conaclos I agree, and this is what would have been my first choice, as it's simple, clear and solves the problem. However, @arendjr requested to do it differently, and @ematipico has approved that approach. Just so that I understand. What could we do differently in the future, so that the solution design doesn't need to change after we have already implemented it? |
@ematipico @arendjr Could you please share your thoughts on @Conaclos suggestion, so that we would know how to proceed with our PR? |
I don't see a problem with that, to be honest :) I think it's perfectly fine to change the logic or design of the rule, while it's still in nursery. I think this conversation is amazing because it's allowing us to understand how to better design the rule based on your feedback. @Conaclos |
Agreed with @ematipico indeed. I will say I don’t mind a I’m unsure about the middle column in @Conaclos ’s table btw. Usually when you bundle or use a runtime that allows direct importing of TS sources, you’re also able to load |
I accept with this point. I am discussing what the option should be: a matrix vs an enum.
The table is only for the current file heuristic: if the current file ends in
I am unsure to see the benefit of the matrix. It comes with extra complexity. However, if you think it is better than a js-ification option (I have no good name for it), then I am good with that. |
So I think the matrix is (slightly) preferred as long as we rely on the current heuristic. For instance, someone may prefer to import from But the enum may be preferred when we have file resolution, because the file resolution should take away such ambiguities. I also agree the advantages of the matrix are minor though (and temporary if we assume the enum is the way to go in the end), so if you feel the complexity isn’t worth it, that’s fine by me. |
I agree with @ematipico that, for now, the best option is to make this configurable. |
Ok, let's go with the matrix. Once we can resolve imports, we will deprecate it in favor of a simpler option. |
Not sure what the behaviour should be when
|
This makes sense of ignoring them indeed. |
Environment information
Rule name
lint/nursery/useImportExtensions
Playground link
https://codesandbox.io/p/devbox/happy-worker-jg4p7k?workspaceId=5b49be67-1204-4d4f-b084-f984140cde23
Expected result
i Explicit import improves compatibility with browsers and makes file resolution in tooling faster. i Safe fix: Add potential import extension .js.
Code of Conduct
The text was updated successfully, but these errors were encountered: