biomejs / biome

A toolchain for web projects, aimed to provide functionalities to maintain them. Biome offers formatter and linter, usable via CLI and LSP.
https://biomejs.dev
Apache License 2.0
15.46k stars 481 forks source link

useImportExtensions should use .js extension for TypeScript imports #2967

Open kibertoad opened 5 months ago

kibertoad commented 5 months ago

Environment information

Currently useImportExtensions rule proposed fix uses a real file extension, which in case of TypeScript is ".ts". This is not correct.
See this discussion:
https://stackoverflow.com/questions/62619058/appending-js-extension-on-relative-import-statements-during-typescript-compilat

Specifically this comment from a person from TS team:
"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)"

Current output:

  i Explicit import improves compatibility with browsers and makes file resolution in tooling faster.  
  i Safe fix: Add potential import extension .ts.

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

minht11 commented 5 months ago

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 .js file file is actually .ts sounds more confusing than helpfull, so in that case I would just disable rule altogether and stick to extensionless imports.

kibertoad commented 5 months ago

Most users are using bundlers

This is only true for frontend. In backend absolute minority uses bundlers.

In source code having import file extension .js file file is actually .ts sounds more confusing than helpful

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)"

Write the paths that work at runtime means using the .js extension for TS files. This is a correct and expected behaviour.

in that case I would just disable rule altogether and stick to extensionless imports.

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 TypeScript refuses to add import extensions at compile time

ematipico commented 5 months ago

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 .ts, that could cause more harm than benefit.

In a bundler world that wouldn't matter, suggesting .ts or .js doesn't change much, but outside of bundlers, suggesting .js would make the rule more useful.

I would just disable rule altogether and stick to extensionless imports.

ESM doesn't work with imports without extensions, and even require. I think we should always suggest .js.

minht11 commented 5 months ago

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.

kibertoad commented 5 months ago

How many people are actually using .js extension in ts files

@minht11 All of them who use TS with ESM. allowImportingTsExtensions is not a solution. 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..

arendjr commented 5 months ago

Also good to mention that using .js to import .ts is limited to transpilation through tsc for Node.js environments only. Both Deno and Bun use regular .ts imports. I’m not sure if Bun allows either version, but at least for Deno it would be an error to use .js imports.

I don’t think all bundlers allow .js imports either, so this would be a clear regression for those use cases.

kibertoad commented 5 months ago

@arendjr Best solution would be to make the behaviour directly configureable within the rule options.

kibertoad commented 5 months ago

@minht11 So what is your preferred path forward, considering that allowImportingTsExtensions is not suitable for transpilation into runtime JS, and .ts is not supported in Node?

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?

arendjr commented 5 months ago

As someone who has been arguing against the TypeScript team's decision to use .js extensions since 2022 (before they stabilized it anyway), I honestly think the entire rationale for using .js imports is severy flawed. Even back then I warned against the ramifications their decision would have on the ecosystem, and here we are, still discussing the aftermath.

If you are in a situation where you need to use .js extensions for your .ts file imports, at least these two conditions apply:

In other words, preferring .js would be a regression for every frontend developer, every Bun developer, every Deno developer, and even many or most Node.js developers, since bundling server-side code is also a very common practice given both its performance and DX benefits. At best it's backwards or confusing, at worst it breaks their tooling.

I wouldn't protest if someone wants to introduce a rule option that caters to the tsc-transpiled Node.js use case, but I do strongly belief the default should be .ts. And ideally, I would just recommend migrating from Node.js or using a bundler instead :)

kibertoad commented 5 months ago

As someone who has been arguing against the TypeScript team's decision to use .js extensions https://github.com/microsoft/TypeScript/issues/49083 (before they stabilized it anyway), I honestly think the entire rationale for using .js imports is severy flawed. Even back then I warned against the ramifications their decision would have on the ecosystem, and here we are, still discussing the aftermath.

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.

If you are in a situation where you need to use .js extensions for your .ts file imports, at least these two conditions apply:

You must be using tsc for transpilation rather than a bundler You must be running the transpiled code inside Node.js

Which basically means you are a backend Node.js developer who uses TypeScript. Absolute minority of backend services use bundlers.

In other words, preferring .js would be a regression for every frontend developer, every Bun developer, every Deno developer, and even many or most Node.js developers

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.

bundling server-side code is also a very common practice

[citation needed]. Anecdotally, I have never once encountered a server-side code using a bundler, outside of SSR frameworks.

I wouldn't protest if someone wants to introduce a rule option that caters to the tsc-transpiled Node.js use case

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 nodeCompatibility: true/false?

arendjr commented 5 months ago

[citation needed]. Anecdotally, I have never once encountered a server-side code using a bundler, outside of SSR frameworks.

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 tsup (or plenty others).

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.

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.

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 --unstable-sloppy-imports and I don't think many will. For Webpack users, it would depend entirely on their configuration whether using the wrong extension will work. Same for users of Rollup or dnt.

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.

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 nodeCompatibility: true/false?

I would opt for a string extension here, so it also allows people to choose .jsx, .tsx or something else if they find the suggestions are off for their particular project.

mrhyde commented 5 months ago

The only viable option here is to rely on the configuration already set in tsconfig (allowImportingTsExtensions). The bundling debate is irrelevant, as it depends on personal preference and project requirements. Generally, you wouldn't want to bundle a published library, whether it's for frontend or backend. Thus, configuring the rule based on tsconfig settings seems the most flexible and practical solution.

kibertoad commented 5 months ago

@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?

mrhyde commented 5 months ago

@kibertoad Why not? If: allowImportingTsExtensions && noEmit == true => set extension to .ts allowImportingTsExtensions && emitDeclarationOnly == true => set extension to .ts everything else => set to .js

kibertoad commented 5 months ago

@mrhyde Oh, that's what you mean. Yeah, that sounds like a great approach.

kibertoad commented 5 months ago

@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

ematipico commented 5 months ago

It's unclear to me what are the next steps to close the issue

kibertoad commented 5 months ago

@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 allowImportingTsExtensions from tsconfig is nice, but is likely to be less universally applicable, as projects can have custom naming for tsconfig, load it outside the root directory, use inheritance etc, so it's not going to be a reliable source in all cases.

ematipico commented 5 months ago

It seems to be a fair fix :)

fox1t commented 5 months ago

Making this rule configurable is the best thing to do. I saw many valid arguments for .js vs .ts. Personally, I never bundle my backend code; I solely rely on tsx for development and tsc for compilation. Adding the .js is the correct call here since it is the TS default behavior and ESM spec, too. Looking forward to seeing this implemented. @kibertoad, if you need help, you can ping me. :)

kibertoad commented 5 months ago

Thank you for the offer, @fox1t! @drdaemos will be the engineer working on it, we'll have in mind that we can ping you if there are some questions :D

fox1t commented 5 months ago

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.

mrhyde commented 5 months ago

@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

fox1t commented 5 months ago

@mrhyde Nice! I prefer having the .js extensions in the TS files, but both approaches work!

Kudos!

Conaclos commented 4 months ago

Reading the last discussions and taking a look at #3274, I think there is something wrong.

useImportExtension relies on the extension of the current file to propose a fix. Ideally we should choose the suggested extension based on the extension of the imported file : Some files can only be imported with their actual extension (.js, ,jsx, .cjs, .mjs), and others can be both imported with their actual extension or a js-ified extension (.ts, .tsx, .cts, .mts).

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). For the time being, we will use the extension of the current file for the choice. Once we are able to resolve imported files, we could switch to the extension of the imported file.

file extension same js-ified
js js js
jsx js js
cjs cjs cjs
mjs mjs mjs
ts ts js
tsx ts js
cts cts cjs
mts mts mjs
kibertoad commented 4 months ago

@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?

kibertoad commented 4 months ago

@ematipico @arendjr Could you please share your thoughts on @Conaclos suggestion, so that we would know how to proceed with our PR?

ematipico commented 4 months ago

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?

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 While your matrix makes sense, we aren't in a position to be able to provide a concrete solution without analysing the project as a whole. Considering what Arend said, it seems there are various set ups, so the best we can do is to accept an option that users can set.

arendjr commented 4 months ago

Agreed with @ematipico indeed. I will say I don’t mind a tsc-specific “js-ification” option either, but it relies more on Biome being able to do file resolution first, which is why I favored the user-provided mapping instead. Currently we have to rely on a hueristic (the current file’s extension) and a mapping has the additional benefit it allows configuration to let tweak the hueristic too.

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 .jsx/.tsx files directly. So stripping the x from the suggestions may not be what we want.

Conaclos commented 4 months ago

Considering what Arend said, it seems there are various set ups, so the best we can do is to accept an option that users can set.

I accept with this point. I am discussing what the option should be: a matrix vs an enum.

So stripping the x from the suggestions may not be what we want.

The table is only for the current file heuristic: if the current file ends in .jsx, then we assume that the imported files are in .js. Of course, once we can resolve imports, we should not strip the x. Also, we could still use the current component heuristic (if the import source is capitalized then we assume that it is a component).

Currently we have to rely on a hueristic (the current file’s extension) and a mapping has the additional benefit it allows configuration to let tweak the hueristic too.

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.

arendjr commented 4 months ago

a matrix vs an enum

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 .js even when inside a .mjs file (because they might only use .mjs for entry points), or they know their components are inside regular .js files (I worked in a codebase like that, and it might even be common with Flow users?).

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.

fox1t commented 4 months ago

I agree with @ematipico that, for now, the best option is to make this configurable.

Conaclos commented 4 months ago

Ok, let's go with the matrix. Once we can resolve imports, we will deprecate it in favor of a simpler option.