microsoft / TypeScript

TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
https://www.typescriptlang.org
Apache License 2.0
101.02k stars 12.49k forks source link

TypeScript cannot emit valid ES modules due to file extension issue #42151

Closed ctjlewis closed 3 years ago

ctjlewis commented 3 years ago

Bug Report

This report is based on the problem covered in #28288 and #16577, but intends to emphasize the fact that this explicitly precludes TS from emitting valid ECMAScript modules.

Issue

Because TS imports do not allow file extensions, but imports in emitted ES modules are not resolved to their relevant file extensions (import ... from './myModule' vs. import ... from './myModule.js'), this output is unusable unless transpiled again with Rollup + Babel or a similar toolkit, no matter how much active care is taken while authoring source code. Executing emitted modules will always throw ERR_MODULE_NOT_FOUND because these sources are not rewritten.

Switching to "module": "commonjs" is not a valid suggestion in this case, as the goal is to get valid ESM output.

⏯ Playground Link

Use yarn test: https://repl.it/@christiantjl/TSImportFileExtensions

💻 Code

tsconfig.json

{
  "compilerOptions": {
    "outDir": "build/",

    "target": "esnext",
    "module": "esnext",

    "moduleResolution": "node",
    "esModuleInterop": true
  }
}

index.ts

import { test } from './importMe';
console.log(test); 

build/index.js

import { test } from './importMe';
console.log(test);

🙁 Actual behavior

The import { test } from './importMe' statement is not modified.

🙂 Expected behavior

build/index.js should contain:

import { test } from './importMe.js';
console.log(test);
andrewbranch commented 3 years ago

Because TS imports do not allow file extensions

They do—index.ts should be written

import { test } from './importMe.js';
console.log(test); 
RGFTheCoder commented 3 years ago

My greatest pet-peeve is that automatic TS imports in VSCode are done as import './importMe' rather than import './importMe.js' by default, which always causes the browser, or node, to error saying it can't find the file. It's really annoying, but at least it changes to .js after you change one of the imports manually.

ctjlewis commented 3 years ago

@RGFTheCoder Yeah, the TS error should be adjusted to say "please use .js file extension when importing TypeScript files" or something. Currently the error presented on:

import test from './test.ts'

is:

An import path cannot end with a '.ts' extension. Consider importing './test' instead.ts(2691)

It is highly unintuitive that you need to use a .js file extension when importing another TS file from inside a TS file, and nobody would really be expected to try that without being explained this first. Ideally the compilation process would just handle it, since that's what it's for: Turning valid TS into valid ES.

It currently turns valid TS into invalid ES. The compiled output does not execute as expected when following the directions provided by TS2691. It is erroneous output.

andrewbranch commented 3 years ago

@RGFTheCoder there’s a setting for that so you don’t have to manually fix up an import for every file:

image

In the future, there will likely be a set of compiler settings that make this the default, and/or make extensionless imports not resolve, but we’re not quite there yet.

Yeah, the TS error should be adjusted to say "please use .js file extension when importing TypeScript files" or something.

Seems reasonable. Not quite with that phrasing, but your PR looks good.

Ideally the compilation process would just handle it, since that's what it's for: Turning valid TS into valid ES.

Starting down this path made me realize I failed to label this as a duplicate of https://github.com/microsoft/TypeScript/issues/16577.

richardkazuomiller commented 3 years ago

I wish this issue could be left open until the problem is solved. There are actual problems with the workaround of just adding .js to the end of the file. Just to name the ones at the top of my mind:

  1. The .js file doesn't exist.
  2. Because the file doesn't exist, ESLint and similar tools will throw an error. I don't know if there's a setting to tell ESLint "Yes, I know that the file I'm trying to import doesn't exist, but I promise that it will later. Please look for a .ts file, or a .tsx file depending on the situation, and if that one doesn't exist, then you can throw an error." If there is, please correct me 🙏.
  3. The TypeScript documentation uses extension-less imports. None of the documentation of TypeScript says "If you don't put .js files at the end of your imports, you can't use ES modules."
  4. There is an elephant in the room, which is that any project that uses modules needs file extensions in the imports. Even if we can't agree on what to do about it, that is the situation we are in, and yet the compiler chooses to ignore said elephant by just outputting files that everyone knows won't run without having some other build step to fix them.
ctjlewis commented 3 years ago

@richardkazuomiller I've considered just building this feature out and throwing it on the table. My last PR related to this was merged very quickly, so the compiler will no longer tell you to do something that actually breaks your emitted output (rename ./myModule.ts import to ./myModule—for ES2015 and up, it now advises ./myModule.js).

I have actually demo'd this logic in my own fork of TSDX and it is working, so I may try to add it directly to the compiler.

The fact of the matter is that "CJS require() will resolve file extensions, but ESM will not, and neither will TS emitting ESM" is not a good state of things and I would not have allowed this problem to go on this long if it were my call.

ctjlewis commented 3 years ago

@andrewbranch I have implemented an algorithm to resolve file extensions AOT. There is no reason theoretically that it could not be optimized and added as an opt-in feature to get valid ESM output.

https://github.com/tszip/rollup-config/blob/master/src/plugins/resolveImports/index.ts

Currently, there is other way of saying it, TypeScript cannot emit valid ES modules.

Reopening Issue

Could this issue be reopened? It was closed when my #42184 PR was merged, I should probably not have added the "closes" syntax since this issue still exists in emitted output unless you specially craft the source to use absolute imports (which is not theoretically necessary since we are eventually feeding the TS compiler our TS syntax, it could just resolve them AOT on an opt-in basis as shown above).

andrewbranch commented 3 years ago

It would have been closed as a duplicate of #16577 anyway. This is not the only issue that laments TypeScript’s lack of proper ESM support. The biggest development on that front is #44501 if you’re using ESM in Node, though much of it will be reusable for other ESM targets like modern browsers. The PR will enforce that you write .js extensions in your imports, and auto-imports will always add extensions (without changing any settings in VS Code).

unless you specially craft the source to use absolute imports (which is not theoretically necessary since we are eventually feeding the TS compiler our TS syntax, it could just resolve them AOT on an opt-in basis as shown above)

This will never be the route TS takes. See https://github.com/microsoft/TypeScript/issues/15479#issuecomment-300240856, https://github.com/microsoft/TypeScript/issues/16577#issuecomment-754941937, https://github.com/microsoft/TypeScript/issues/26722#issuecomment-580975983, https://github.com/microsoft/TypeScript/issues/33588.

thetutlage commented 3 years ago

@andrewbranch Just trying to understand the TypeScript take on ES modules, maybe you can help shed some light.

As per me TypeScript compiles to JavaScript that the JavaScript engines can run. I can control the output I want (via tsconfig.json) and then TypeScript will compile accordingly. For example: If I compile for ES3, then TypeScript will re-write my classes to something an ES3 JS engine can run.

Then what is different about ES modules? Why it cannot compile it to something that the JS engine can run? I mean it is already modifying my import statements to require calls when I ask it to compile for CJS

ctjlewis commented 3 years ago

@thetutlage Sadly this team has made it very clear where they land on it—I 100% agree with you, it literally does not emit valid ES modules and as you also pointed out, transpiling is no problem when it comes to backwards compatibility, but when it comes to supporting ES2015 (is really what we're talking about here, just actual functional ES6 imports in emitted output), it is somehow off the table. It has pretty real consequences, as an unfathomable amount of source code is locked into CJS de facto because of that design choice.

Some packages, like React IIRC, even ship the "BabelScript" TS emits as the ESM entry point, leaving coercion to actual, valid ES to the downstream consumer. This pointlessly gives up a lot of the benefits that static ES module resolution affords by refusing to rewrite TS-like import specifiers to actual ES import specifiers, and requiring all emitted code to be transpiled, often to CJS, in order to run it (preventing us from having our cake and eating it too).

I do not see how there is any cost to adding an opt-in flag so that we can get valid ESM out of the compiler—despite the extensive clarification, I still think it's the wrong call. The standard is five years old and I think output should run when we execute it, even if we have module: 'esnext' set.

So, I also disagree with the choice, but it seems like ecosystem tools are the way to go for now. See @tszip/tszip which, at its core, mostly just executes the resolution logic we're talking about here over TSC output and rewrites invalid relative directory imports emitted by TypeScript to full file specifiers. I encourage everyone to check the resolution logic in @tszip/resolve-imports regarding this issue.

richardkazuomiller commented 3 years ago

This makes me sad. If I shipped a library and said that all projects using that library need a separate plugin whose only job is to append three characters to a string, without which the program can't in any environment, and my library doesnt mention it in the documentation or show any warnings about it, I think nobody would want to use my library 😂

andrewbranch commented 3 years ago

Nobody needs to use a separate plugin to rewrite their paths. All module resolution modes allow you to include the .js file extension on your paths: valid ESM in, valid ESM out. The unfortunate situation we’re in with --module esnext is that we also allow invalid ESM in, invalid ESM out. We are fixing this with --module node12 / --module nodenext and I would expect that eventually, we will have a mode that more accurately represents non-node ESM environments (like modern browsers) as well, though I can’t speak to any specific plans or timelines.

Then what is different about ES modules?

Transforming an import declaration to a require is a simple syntax transformation that doesn’t depend on outside environmental information like what files the compiler found on disk. Transforming a module specifier of "./foo" has to depend on module resolution results, otherwise you don’t know whether to write "./foo.js" or "./foo/index.js". Even if we wanted to do this (and we don’t), we would have to disallow it under --isolatedModules, because single-file transpilation cannot possibly know what to do with those module specifiers.

I mean it is already modifying my import statements to require calls when I ask it to compile for CJS

I’m not speaking for the team as a whole on this, but personally I hate this. I think we’d be in a less confusing place today if we made people who want CJS write CJS and people who want ESM write ESM. But that ship sailed long ago.

ctjlewis commented 3 years ago

All module resolution modes allow you to include the .js file extension on your paths: valid ESM in, valid ESM out.

Yes, but now you're writing "invalid" TS—it's technically valid because it was added as an ad hoc, so the compiler won't throw, but ./myModule.ts is literally not ./myModule.js, and if you build to a bundle, myModule.js might not exist at any point—so we're just mixing everything up and using antipatterns because the TypeScript team has made the (wrong IMO) choice to not rewrite these imports at compile-time while still allowing you to specify them that way (which you pointed out).

Nobody thinks to go, "oh let's specify the module with a .js extension even though it doesn't exist¨—the compiler finally tells you to do that after I added it in #42184, but only if you try to reference it via ./myModule.ts. My honest impression is that the ball has been dropped on this, which is pre-empting the ecosystem from adopting a 5-year-old standard because so much shipped JS is written in TS source.

fregante commented 3 years ago

people who want CJS write CJS and people who want ESM write ESM. But that ship sailed long ago.

Deprecations exist, especially if we get decent support for ESM today in exchange.

richardkazuomiller commented 3 years ago

All module resolution modes allow you to include the .js file extension on your paths: valid ESM in, valid ESM out.

If this is the only way to get valid ESM, then shouldn't all of the TypeScript documentation have .js at the end of all the imports?

thetutlage commented 3 years ago

Transforming an import declaration to a require is a simple syntax transformation that doesn’t depend on outside environmental information like what files the compiler found on disk. Transforming a module specifier of "./foo" has to depend on module resolution results, otherwise you don’t know whether to write "./foo.js" or "./foo/index.js".

TypeScript already knows where ./foo is, since it is giving me type information about the module. So it should be able to write it to the correct destination with required modifications.

Even if we wanted to do this (and we don’t), we would have to disallow it under --isolatedModules, because single-file transpilation cannot possibly know what to do with those module specifiers.

If single file modules are supported as first class citizen, then again TypeScript should rewrite that import the way it should be inside a single file.

The main point of using the transpiler is not to worry or write code as per the output. Write it in one uniform way and let the config + compiler deal with the rest.

What if I want to use TypeScript to output CJS and ESM both. Which standard should I author my code in?

thetutlage commented 3 years ago

Also, I think the debate is getting diverged towards specifics. But it is not about specifics, it is more about a generic broader goal of TypeScript.

If a compiler claims to output JavaScript from some superset of JavaScript, then it should let the user write code in that superset and not ask them to mix-match JavaScript with superset.

ctjlewis commented 3 years ago

Deprecations exist, especially if we get decent support for ESM today in exchange.

This is true, but nothing needs to be deprecated, we literally only need optional import specifier rewrites. I appreciate that the TypeScript team has thought about this a lot and has made a decision, but I strongly believe it is the wrong one and it's never too late to make the right call. I have seen the work on Node12 module resolution, and it is fantastic, but it does not solve the problem of allowing maintainers to compile existing projects to "complete ESM" (ES modules which are valid ES programs according to the spec, i.e. refer to absolute import specifiers), without going and rewriting all of their imports to reference a-package/path/to/module/index.js instead of a-package/path/to/module, by hand.

Much of today's JS is compiled TypeScript, and therefore locked into the limitations of the TS compiler. As it stands, the compiler is voluntarily incapable of turning today's or yesterday's TypeScript into valid ESM, even if you were willing to go out of your way to ask it to, despite this being feasible in reality. This limitation is imposed on every TS project, and in practice, locks projects into CJS, or this incomplete ESM (which I affectionately call BabelScript, since it must be transformed again before it can be executed). The best we can do is post-processing to coerce that incomplete ESM to full ESM, but everyone who doesn't do that is usually back-transpiling to CJS directly (via module: commonjs) or indirectly (via downstream Babel transform on the incomplete ESM).

Thus, not adding an opt-in flag that lets developers elect to emit valid ESM from normal, legacy TypeScript—as it is written today, not Special Node12 TypeScript with absolute import specifiers ending in .js, but existing libraries with TS-like imports that could be resolved AOT via an opt-in --resolve-esm-imports flag—limits presumably billions of lines of code from actually upgrading to ESM, even if they really really wanted to.

Sorry to Andrew and other team members for repeatedly pressing this issue, I just believe it is possible for us to have our cake and eat it too in this case.

If a compiler claims to output JavaScript from some superset of JavaScript, then it should let the user write code in that superset and not ask them to mix-match JavaScript with superset.

When you're right, you're right—I think the Node12 resolution is a good addition but it basically requires that you trade away TS import specifiers completely, which may be a fine default for Node12, but there's clearly massive utility in letting developers choose. No need to force new defaults on anyone.

The "mixing JS and TS" principle is also why it's ultimately insufficient IMO, because it would require you rewriting all the imports in your source (not just TSC rewriting them in emitted output), by hand, in order to get existing codebases to compile to actual ESM.

ctjlewis commented 3 years ago

Sorry again to drag this out @andrewbranch, but is there a clear reason why catering to Svelte's compiler is appropriate with #44619, but supporting standard ESM as an opt-in flag is ruled out completely? I am struggling to understand the logic here.

orta commented 3 years ago

I'm not really sure that comparing an example of continuing the TypeScript-specific import type syntax in #44619 (which is useful everywhere you want to safely erase types without type system checks, which includes Svelte but also any other TS -> JS transpiler) is comparable to TypeScript breaking one of its core design tenets as has been mentioned a bunch of times above.

thetutlage commented 3 years ago

@orta Till date I am not able to understand when writing TypeScript, should I be writing the code by keeping the compiled target in mind? If yes, then why for years I was writing import and getting require in the output.

richardkazuomiller commented 3 years ago

then why for years I was writing import and getting require in the output.

Exactly, could someone explain like I'm 5 why it's OK for TypeScript to convert import ... from './file' to const ... = require('./file') changing ./file to ./file.js is not OK?

thetutlage commented 3 years ago

typescript-nonsense

^^ I think this is how the decision making is done.

richardkazuomiller commented 3 years ago

Thanks. I think I understand the technical distinction between the file path and the other parts, and at least some of what makes it difficult to implement, but what I don't understand is the philosophical reason why people don't want to do it. Like for example, I understand that ./foo might refer to ./foo.ts or ./foo/index.ts and that makes things more complicated than they are now, but I still don't understand why that's so bad.

orta commented 3 years ago

Feel free to read up on the links provided in https://github.com/microsoft/TypeScript/issues/42151#issuecomment-914472944

Especially https://github.com/microsoft/TypeScript/issues/16577#issuecomment-754941937 - but if this thread is just going to keep pulling it back to that topic instead to the actual point of the issue then we'll end up having to lock it also.

thetutlage commented 3 years ago

Well, it is your wrong assumption that we haven't read those comments. Infact, it is the other way around. No one from the TypeScript team is yet able to explain the thesis behind this design choice.

Atleast, at a high level, tell me in which language am I supposed to author my code. Is it TypeScript or the compile target?

Yes, you have all the rights and power to lock issues, but still the question won't be answered.

orta commented 3 years ago

I assume you are writing .ts files which are TypeScript. TypeScript follows the JavaScript language spec and adds its own syntax for types. No-one really writes TypeScript in a vacuum, because it needs some kinda of JS runtime environment to actually be evaluated - so the two are linked.

Your argument is about taking a JavaScript language feature import/export and backporting the semantics of that to work with a CommonJS runtime. This is the same concept as how we backport something like private class fields if your target does not support it (e.g. via a WeakMap on older runtimes). That is taking a standardized language feature of JavaScript and making it work for your target environment.

As an example, you should note that we don't do CommonJS to UMD, AMD or SystemJS because that's not taking a JavaScript language feature and porting it to an environment - that's the sort of features bundlers have.

I'm afraid it is not the same as changing string identifiers in the JavaScript code during emit.

thetutlage commented 3 years ago

Okay. So is it safe to assume that JavaScript language features will not be ever modified by TypeScript. For example:

orta commented 3 years ago

Yep, all of that has/will happen.

TypeScript follows the JS spec, it doesn't go off and do its own thing anymore - those decisions were made in a very different JS ecosystem. There will be flags for keeping the old behavior when things don't match like decorators and enums, and private x will always have different behavior from #x. People have been writing import/export the way it was spec'd for years in TypeScript now and that's not probably going to change now that it's in the spec.

thetutlage commented 3 years ago

Cool. Thanks for explaining. This makes it easier for me to explain why things are the way things are

richardkazuomiller commented 3 years ago

I understand that this is an intentional choice, but I don't think most people do. I just took a quick peek at Discord and people are still asking questions about this several times a month, which is not surprising because of the reasons we've already mentioned. The de facto standard of TypeScript is still to not write the extensions in imports, and the average person doesn't know that they should be adding .js at the end of everything. If I copy code from the documentation for modules and try to output it to ES modules, it won't work. There is actually an example with the heading "Native ECMAScript 2015 modules SimpleModule.js" but the example will not run because it doesn't have the .js extension, and there is nothing written about the fact that this output will not run.

import { something } from "./mod";
export var t = something + 1;

I think when I asked why the documentation doesn't have .js in the imports, it came off as rhetorical or me trying to be snarky, but I'm honestly confused about why putting the path including the extension to the .js file which will exist post-compilation is not being pushed as the best practice and there is an example in the documentation that doesn't work. More and more packages are moving to ESM-only, which means more and more people will have to move their projects to ESM, and ask some version of the question, "What do you mean I have to write .js? I'm using TypeScript."

orta commented 3 years ago

That's a great issue to put on the website, but until 4.5 TypeScript didn't support node's esm, so everyone using it ESM with TS happened to have it work through some good luck and that the classic resolver acted very similar to how node's ESM mode worked out of the box. ESM support in node is very new, has been changing a lot and has only recently been stable across most versions is simply the answer.

Most people are still writing commonjs in Node though so the docs probably will still represent that, debating about when we should switch is a good question for the website repo.

ctjlewis commented 3 years ago

Your argument is about taking a JavaScript language feature import/export and backporting the semantics of that to work with a CommonJS runtime. This is the same concept as how we backport something like private class fields if your target does not support it (e.g. via a WeakMap on older runtimes). That is taking a standardized language feature of JavaScript and making it work for your target environment.

Yes. How does this logic not apply to the situation where I write a valid TS import that the compiler will emit as an invalid import specifier? It does not work. It needs to be rewritten and "backported" to the target. It is very straightforward.

It seems very obvious that this is a needed feature, which is evidenced by people coming into this issue nearly a year after I brought it up with this team. We are just getting all the downsides of corporate bureaucracy (making a bad design choice and double/triple/quadrupling down for arbitrary, abstract reasons) with none of the upsides (like output that conforms to the 5-year-old specification) here.

Worse even, as others mentioned, it is insanely confusing for newer developers (for very good reasons—it should "just work") and has all sorts of downstream impact in terms of CJS lock-in.

I'm afraid it is not the same as changing string identifiers in the JavaScript code during emit.

It literally is when the import specifier is not valid ECMAScript unless it's rewritten! Polyfills are fine in all other instances except this one apparently. The import specifier in the emitted code will not execute correctly, so the only decision is whether or not it is appropriate to give developers the ability to force rewrites.

Most people are still writing commonjs in Node though

Good amount of them are locked into it de facto because of this issue.


At this point I'm just going to write the feature and make you guys close the PR. This is ridiculous.

andrewbranch commented 3 years ago

I welcome thoughtful discussion and debate, but this conversation is running in circles and getting heated. I apologize to those who were just asking questions in good faith, but some others in this thread are going to have to find a more productive way to engage in conversation about decisions they disagree with. This issue is a duplicate of many others, linked throughout the comments above. We have been talking about this for years, and nothing new has been said in this thread, so it has become just a drain on maintainer time and energy. Thank you for understanding.