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

Offer option to resolve module path maps in emitted code #26722

Closed dannycochran closed 4 years ago

dannycochran commented 6 years ago

Search Terms

paths tsconfig.json compilerOptions ts-node node cannot find module

Suggestion

Per #10866, module path maps are not resolved in emitted code. This means that if you attempt to run the emitted code with node, it will be unable to resolve those paths and the server will not start.

Current solutions:

  1. use browserify or webpack to bundle the output -- this is an unfortunate solution given that it'd be the only reason for me to bring in one of these tools, as I've managed to build and deploy apps using just the typescript compiler. This blog post from the TypeScript team even strongly recommends using a TypeScript toolchain:

https://blogs.msdn.microsoft.com/typescript/2018/08/27/typescript-and-babel-7/

For that reason, we feel tsc and the tools around the compiler pipeline will still give the most integrated and consistent experience for most projects.

  1. use a tool like module-alias. This is a bit gross as it requires duplicating the path maps, and inserting code at the root of your server.

Proposed solution:

Offer a compilerOption, something like resolvePaths, that resolves aliased paths to their relative paths.

Use Cases

So that I can continue to meaningfully use the paths property in tsconfig.json.

Examples

Please see the example in #10866.

Checklist

My suggestion meets these guidelines:

arsamsarabi commented 5 years ago

Thanks for you replay @RyanCavanaugh. Would you mind pointing me to one of the places where I can read up on the ideas behind module paths?

RyanCavanaugh commented 5 years ago

https://github.com/microsoft/TypeScript/issues/26722#issuecomment-524375687

RyanCavanaugh commented 5 years ago

There's also primary design goal number 7: Preserve runtime behavior of all JavaScript code https://github.com/Microsoft/TypeScript/wiki/TypeScript-Design-Goals

An import statement is JavaScript code, so unless it needs downleveling, per our design goals, it should be left alone.

alexandertrefz commented 5 years ago

Frankly, design goal 7 seems to indicate the exact opposite reaction. You have set up compiler config, you are changing your code for that compiler config, now the compiler is breaking your JavaScript and thus changing your runtime behaviour. Thats exactly why everybody wants these paths rewritten. Such a simple change, with so much fuss, is really a shame.

I appreciate all the efforts of the Typescript team, but the given reasons for preventing a change that is comparatively speaking so trivial and has, again relatively speaking, so little possible negative impact while such a big part of the userbase is asking for it is just baffling to me.

kitsonk commented 5 years ago

You have set up compiler config, you are changing your code for that compiler config, now the compiler is breaking your JavaScript and thus changing your runtime behaviour.

The only situations that it breaks behaviour is when you never had a valid behaviour to begin with. It just isn't magically fixing it for you, and that upsets you.

paths was introduced to mimic the potential runtime behaviour of different loaders, so you could write your TypeScript in a way that would match the runtime configuration.

I appreciate all the efforts of the Typescript team, but the given reasons for preventing a change that is comparatively speaking so trivial and has, again relatively speaking, so little possible negative impact while such a big part of the userbase is asking for it is just baffling to me.

Then make a proposal that would work. A proposal would include what gets emitted and the logic. People so far have only proposed that some sort of magical flag be added to the configuration that will read the developers mind and emit something, but haven't expressed some sort of valid emit.

RyanCavanaugh commented 5 years ago

Frankly, design goal 7 seems to indicate the exact opposite reaction. You have set up compiler config, you are changing your code for that compiler config, now the compiler is breaking your JavaScript and thus changing your runtime behaviour. Thats exactly why everybody wants these paths rewritten. Such a simple change, with so much fuss, is really a shame.

Our position is that you should write the path that works at runtime, and set up your config however is necessary so that TS can understand what those paths resolve to. What's being requested is the opposite: that the developer writes a path other than the one that works at runtime, and that the config would tell TS what path to write instead. That's exactly what's being proposed in this comment, for example.

It seems like what several people in this thread have done is:

The correct solution is to just stop at step 1 where everything worked and you were writing the path you wanted at runtime! Which seems to be exactly what you're saying in your comment.

Am I taking crazy pills here? "TS doesn't have its own internal symlinking system" is a valid observation but it's not the case that we're trying to break your program by forcing you to use an invented set of fictional paths that never worked in the first place.

alexandertrefz commented 5 years ago

The only situations that it breaks behaviour is when you never had a valid behaviour to begin with. It just isn't magically fixing it for you, and that upsets you.

I had a whole thing written here, but @RyanCavanaugh has put it much more succinctly.

You are correct, this is what most people have done here, because paths are primed for this. Everybody who doesnt want to use the awfulness that is most tools that surround JS compilation at this day and age just want to compile with TS directly, and would just love paths, and then there they are, staring you straight in the face but refusing to take the last and easiest step. And the first time you notice, is after you have made it all compile.

And frankly, clearly this is how many people not only want to use this feature, but intuitively assume that this is exactly what it is for.

I wouldnt say you are taking crazy pills, but why not simply enable this trivial feature, when there is so little downside? That seems crazy to me!

But lets get to @kitsonk`s suggestion:

After writing all this i realised that i have basically rewritten a proposal from way up in the thread, but hopefully in a clearer way, with an added example.

I will say that I have a little experience with the complexities of compilers and compiler options for programming languages, but nowhere near the amount you guys have, obviously. But i completely fail to see how this proposal add any significant complexities to thinking about the language or the compiler, as mentioned here

And while I dont condone the tone people have taking in the later portions of this thread, I can understand and share their frustration that a simple and easy to implement proposal is getting such blanket refusal, on what, respectively, seems so very little reasonable grounds.

Anyways, here goes nothing.

A simple proposal

Disclaimer

First off, I have not seen(which is not to say it doesnt exist) any reasons why this would not work, so im outlining the most simple and least intrusive thing I can think of to make the most basic case work nicely that would cover what I think is 99% of what the people in this issue yearn for.

The import paths can already be resolved internally in TS, otherwise it could not type check, which means the logic for checking wether an import path has a matching rewrite in the tsconfig is there, and working.

Terms

Changes

Add a singular flag --rewritePaths which changes emitting behaviour such that an import statement with a ResolvedPath is using that path for emitting. It should change .ts file endings to .js.

It defaults to false.

All other behaviour remains untouched. If no paths are set, the flag becomes an effective no-op. This is only in effect if outDir is being used of course, if everything gets turned into a single file, imports are not relevant. This behaviour should not be impacted by any other compiler config. AFAICT.

Example

All following file paths are of course relative to the --baseUrl.

Assuming paths to look like this:

"paths": {
    "helpers": ["helpers/index.ts"]
}

This file, located at "library/complicated/deep/path/source.ts"

import { helper } from "helpers"
helper()

should emit as

import { helper } from "../../../../helpers/index.js"
helper()

if paths was empty, it would emit as

import { helper } from "helpers"
helper()

Please let me know about any issue or oversight, I'm happy as can be to have productive discussion about this.

Also a final disclaimer: it is rather late over here in germany, so forgive me for typos or bad phrasing.

dannycochran commented 5 years ago

@RyanCavanaugh You are not taking crazy pills, but I think the hangup is here:

Add path mapping to their tsconfig to invent new module paths that don't work at runtime (e.g. @Component/foo)

It does work at run time if you're running with ts-node, which is a fairly common way of running TS code locally before shipping to a deployed environment, where it's compiled and no longer works. I can't really think of another place with TS compilation where the compiled code cannot execute because of runtime errors that otherwise don't occur when using ts-node -- maybe if you set the target too high for a node or browser environment?

@kitsonk I don't think these comments are totally fair given some of the previous communication from the TS team:

Being a complete build tool is not a goal of TypeScript. tsc is not a bundler, never will be a bundler (or a full featured build tool)...

From this blog post:

Using the TypeScript compiler is still the preferred way to build TypeScript. While Babel can take over compiling/transpiling – doing things like erasing your types and rewriting the newest ECMAScript features to work in older runtimes – it doesn’t have type-checking built in, and still requires using TypeScript to accomplish that. So even if Babel builds successfully, you might need to check in with TypeScript to catch type errors. For that reason, we feel tsc and the tools around the compiler pipeline will still give the most integrated and consistent experience for most projects.

I don't think it's unreasonable for folks to expect that tsc is the only thing they need to build and deploy code. If the TS team strongly disagrees, I'm not sure they've sufficiently marketed that idea.

Then make a proposal that would work. A proposal would include what gets emitted and the logic. People so far have only proposed that some sort of magical flag be added to the configuration that will read the developers mind and emit something, but haven't expressed some sort of valid emit.

I'm not sure I understand what's problematic with this example: https://github.com/microsoft/TypeScript/issues/26722#issuecomment-524405693. I added a tsconfig.json-- you mentioned you weren't sure how the @common would be resolved, sorry I had included that part in a previous comment.

RyanCavanaugh commented 5 years ago

Here are some questions I have about this feature:

Again though, I still consider this a non-starter feature, because the statement

import a from "b";

is an ECMAScript statement, and per design goal 7, when it doesn't need to be downleveled, we do not change it. This is no different from saying TS should cause a runtime error when a divide-by-zero occurs: We don't change the semantics of JS code.

eyedean commented 5 years ago

I think at this point it's obvious that we have a philosophical problem, and NOT a technicial one. : )

In fact, my technical proposal has been sitting here for 3 months now receiving tens of upvotes from users. In the meantime, the third-party solution for this exact issue has passed 500,000 downloads per week earlier this month!


It seems like what several people in this thread have done is: • Start with working relative paths • Add path mapping to their tsconfig to invent new module paths that don't work at runtime (e.g. @Component/foo) • Change their files to import from these fictional paths • It doesn't work at runtime because the runtime path is invalid • Feature request: TS should resolve my fictional paths for me

The correct solution is to just stop at step 1 where everything worked and you were writing the path you wanted at runtime!

I am sorry @RyanCavanaugh, but I smell the same dictatorship that another user mentioned in this thread earlier. I don't think when there is such a simple request, with millions of downloads for an alternative per month, you can tell what people fictionally WANT is WRONG. That's not how technologies grow and gain popularity.

Putting all the details behind, developers simply want to have absolute paths in their imports instead of a train of ../../../.. that breaks so many things if you move a folder. That's as simple as it.


All being said, what we have heard so far is:

Case A) People shouldn't do it

I'm sorry, just because you are in charge of official team behind an open source project it doesn't mean you can dictate people what they are allowed to WANT, if it's a valid and sane popular request. (Reminds me of the same stance Microsoft had in early 90's, which led to the birth of Open Source and Linux!)

Case B) It doesn't fall under the responsibilities of tsc

I'm sorry, but tsc already handles it during compile/type-checking time very well using paths. In fact, as @dannycochran mentioned above, if you guys want developers to rely on tsc as the preferred way to build, please make life easier for everyone.

Otherwise, the brand behind TypeScript would become "there is an official tsc compiler, but it doesn't include what it takes to turn your working .ts to working .js flawlessly, unless you download this and this and that third-party package."

Case C) Oh, if we do this, then we need to do that, and rewrite half of the compiler

No! All people, and I am talking about millions of people, want is SUCH A SIMPLE CAPABILITY TO BE ADDED UNDER AN OPTIONAL FLAG. Optional means (I think I ought to mention it) that it's going to be off/disabled by default. So unless someone actually goes and turns this new knob, it's not going to do anything. Thus, not a breaking change.

We can sit and argue for eons, but at the end of the day, Done is better than Perfect! And here, undone (plus the attitude we are seeing here) is hurting the reputation of a growing technology.

Thanks.

PS.

Here are some questions I have about this feature:

I am not sure if these are questions to improve the comprehensiveness of the propsal, or just to show off that oh it's difficult/unwanted-by-the-official-team.

Let's stay positive, anyway! Instead of going into the weeds, I can tell that the User Story is as simple as: I have @model/*" : [ "src/model/*" ], under paths in my tsconfig.json. I just want any import userData from "@model/user/data" to work as expected from anywhere in my codebase. Currently it works during compile-time but doesn't work during run-time, as you know.

I bet this is 98% of what people (at least those millions of customers of the third-party) want. It's up to you to decide whether these 98% customers should wait until we get the answer to all the little details of what the other 2% want or not.

Thanks for your time anyway.

RyanCavanaugh commented 5 years ago

All being said, what we have heard so far is: ...

Many of those things have not been said.

Let me try to be as clear as possible:

It's not even about scope, it's about one of the ten things we said we will never do, and we intend to keep not doing it. People have told me dozens of times that we need to violate those principles for their particular feature or else TypeScript is doomed to failure.

I am not sure if these are questions to improve the comprehensiveness of the propsal, or just to show off that oh it's difficult/unwanted-by-the-official-team.

People keep shouting about how this is trivial, therefore must be done, as if we're afraid of doing the work. It may be simple to implement, but that doesn't mean it's conceptually straightforward, and ultimately we care a lot more about the conceptual load of a feature that our users have to endure than the difficulty of putting together a PR to add the feature.

ark120202 commented 5 years ago

Wouldn't it also conceptually conflict with import maps?


Here are some questions I have about this feature:

Another one I'd like to add is handling of dynamic import syntax. Should they be transformed as well? If yes, that might present a very surprising behavior:

import('path'); // works
const path = 'path';
import(path); // fails

If no, that might be against the specification, since both static and dynamic imports use the same abstract operation for specifier resolution.

kitsonk commented 5 years ago

Wouldn't it also conceptually conflict with import maps?

As the paths feature stands now, it reasonably models import maps. It is how the feature of SystemJS and AMD was standardised in a browser context. It doesn't conflict conceptually any more because of it, but it underlines that TypeScript doesn't want to get into the business of re-writing module specifiers, because that is effectively a runtime only concern.

wintercounter commented 5 years ago

As I mentioned before the solution would be an overhaul to the plugin system. Another pain point which the community has complaining about for years now. It would save a lot of headaches fro you guys if we would be able to customize the compiler on our own through plugins and it would easily tackle such cases as this one.

On Fri, Sep 20, 2019, 04:04 Kitson Kelly notifications@github.com wrote:

Wouldn't it also conceptually conflict with import maps?

As the paths feature stands now, it reasonably models import maps. It is how the feature of SystemJS and AMD was standardised in a browser context. It doesn't conflict conceptually any more because of it, but it underlines that TypeScript doesn't want to get into the business of re-writing module specifiers, because that is effectively a runtime only concern.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/microsoft/TypeScript/issues/26722?email_source=notifications&email_token=AAHLJQGE4HWI2OCOPAL6HU3QKQVUPA5CNFSM4FSCEI7KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7FKBWY#issuecomment-533373147, or mute the thread https://github.com/notifications/unsubscribe-auth/AAHLJQEH4Q6YKFQ5RXLVEC3QKQVUPANCNFSM4FSCEI7A .

tiagonapoli commented 5 years ago

+1

mrmckeb commented 5 years ago

I think this is really important, as TypeScript declarations for libraries are essentially broken if you use absolute paths. We then need to manually fix them, or use a tool to go through and correct paths.

If my library internally imports ./src/folder as folder (which is the same as ./folder), this works fine. I can use Rollup, Webpack, etc. to update the paths as appropriate.

But I can't do the same for d.ts files. The result is that TypeScript (in a user's project) thinks my library is importing a package called folder and fails.

For the JavaScript itself, I understand the argument for not changing output.

georgyfarniev commented 5 years ago

I spend all day trying to figure out what wrong with my setup and why paths aren’t resolved in outputted js files. Makes this feature almost useless and very confusing, since it processes NOT WORKING output for NodeJS. Please add some option to resolve paths that tsc already knows internally and bring the end to this struggles. It wouldn’t break anything if it will require option to enable resolution.

mrobrian commented 5 years ago

I also ended up here after searching for why my emitted code doesn't have the paths I expected. After reading through this thread, I think I agree with what seems to be the unpopular position of not changing the paths in the output. There's just too much to consider, and I don't see how tsc could possibly account for every variation of build process out there, not to mention things like automated tests, etc.

What I would suggest to the tsc devs is to make the documentation a little clearer here: https://www.typescriptlang.org/docs/handbook/module-resolution.html#path-mapping

Right now, that section could be interpreted to mean that emitted code will have the paths resolved, but really the setting just makes it so that we can use other tools for module resolution and allow TypeScript to understand what those imports mean. The documentation not being clear about that seems to have led to some confusion and expectations among a lot of users, myself included.

eyedean commented 5 years ago

There's just too much to consider, and I don't see how tsc could possibly account for every variation of build process out there, not to mention things like automated tests, etc.

@mrobrian what is suggested here multiple times (and has received many upvotes and support from other developers) is to add an optional flag that supports the simple absolute paths to relative conversation in the output, just as it's already doing in the compile time.

The two bold words here are:

Thanks.

tonitrnel commented 5 years ago

+1

MikeMitterer commented 5 years ago

Just gave up and change 72 file in one of my packages. Moved back from module path to relative path imports. IT LOOKS SOOOO UGLY - All those ../ It makes me wanna puke! It is tilting at windmills - I'm broken. MS you won - Congrats! :cry:

t83714 commented 4 years ago

Here are some questions I have about this feature:

  • What happens when an import resolves to a .d.ts file? Should these paths be rewritten too?

    • Should it be possible that importing a/b and a/c would write a/b but not a/c as a result?
  • Does this apply to all imports that start with a non-relative name, or just those where path mapping was invoked?

    • Does a path mapping that only changes the case of a path qualify under the above?
  • What if I only want to rewrite some of my paths?
  • Should this treat symlinks as opaque, or transparent?
  • Does this apply when an import resolves to an ambiently-declared module, but that import path also has an applicable path mapping?

    • If "yes", this means adding a .d.ts file to your compilation could break the runtime output of your program. This is unprecedented.
    • If "no", then how does a user predict whether a given import will be rewritten?
  • Should paths in emitted .d.ts files get rewritten? Why or why not?

    • What about reference directives?
  • What happens if an import resolves to a .ts file in node_modules? Should TS write out ../../node_modules/foo/index.ts ?

    • If not, by what logic is this excluded?
  • What happens under --allowJs when an import resolves to a .js file?

Again though, I still consider this a non-starter feature, because the statement

import a from "b";

is an ECMAScript statement, and per design goal 7, when it doesn't need to be downleveled, we do not change it. This is no different from saying TS should cause a runtime error when a divide-by-zero occurs: We don't change the semantics of JS code.

@RyanCavanaugh I think what people ask for could be a rather simple solution and has nothing to do with the existing compile-time module resolve process. I think what people want is:

Moreover:

I think the simple logic above should avoid most of the issues you mentioned in your post as this module name rewritten logic is triggered by string match and processed by string replacement without any module resolution process involved.

t83714 commented 4 years ago

It also achieves what we ask for using babel & babel-plugin-module-resolver (just in case anyone looks for alternative solution):

Unfortunately, babel won't touch *.d.ts files in the dist directory.

It will be good that this feature can be implemented by tsc or make it possible to create a plugin of tsc to do so.

tconroy commented 4 years ago

Curious if anyone has found a solution for rewriting import alias in *.d.ts files? We're using babel to build TS to JS which resolves the alias correctly, but it's broken in *.d.ts.

wintercounter commented 4 years ago

Yes, https://www.npmjs.com/package/tscpaths UPDATE: Sorry, linked wrong package first.

tconroy commented 4 years ago

@wintercounter Hm. Doesn't seem to work if your tsconfig.json extends from another config. In my project, we extend from a dependency config and tscpaths blows up.

wintercounter commented 4 years ago

The author did a limited implementation just to support the basics, but I think it'd be easy to add support for extends. Make a PR if you have time.

On Wed, Jan 8, 2020 at 5:00 PM Tom notifications@github.com wrote:

@wintercounter https://github.com/wintercounter Hm. Doesn't seem to work if your tsconfig.json extends from another config. In my project, we extend from a dependency config and tscpaths blows up.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/microsoft/TypeScript/issues/26722?email_source=notifications&email_token=AAHLJQBLU7OCXGYZOC6C2ITQ4X2BDA5CNFSM4FSCEI7KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEINBRAA#issuecomment-572135552, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAHLJQHWFLAQHJPBTQQK4RLQ4X2BDANCNFSM4FSCEI7A .

shlomiassaf commented 4 years ago

After reading a heavy portion of this thread I can say that everyone is right, from their perspective.

What I (again. I) think the problem in this discussion is?

TS users come from a working mode of, hey, i'm running my node app with TS in development (ts-node/webpack/whatever) and it works!!

Now, when I build it does not!

While (I guess) TS authors look at it differently, TS is just a platform to take on form of source code and output another form of source code and having it run as intended.

This came to bloom mainly because of the gaining popularity of monorepos and how easy it is to build libraries that way, alongside a working application.

Now, there are 2 scenarios:

1) The app and libraries are both compiled to a DIST folder and run from it 2) The app is compiled to a DIST folder, the libraries are compiled but published to a package repository (e.g NPM)

With (1), users expect TS to take those paths mappings and convert them to relative paths so they will work cause they are not in node_modules....

With (2) user's will not expect that to happen because it will cause the imports to point to nothing.

I understand TS's team take on this, it is something that will work with (1) but not (2) and that's enough to rule it out.

Maybe i'm wrong, but it seems to me it's one of the reasons for the mixup

eyedean commented 4 years ago

After reading a heavy portion of this thread I can say that everyone is right, from their perspective.

What I (again. I) think the problem in this discussion is?

TS users come from a working mode of, hey, i'm running my node app with TS in development (ts-node/webpack/whatever) and it works!!

Now, when I build it does not!

While (I guess) TS authors look at it differently, TS is just a platform to take on form of source code and output another form of source code and having it run as intended.

This came to bloom mainly because of the gaining popularity of monorepos and how easy it is to build libraries that way, alongside a working application.

Now, there are 2 scenarios:

1. The app and libraries are both compiled to a DIST folder and run from it

2. The app is compiled to a DIST folder, the libraries are compiled but published to a package repository (e.g NPM)

With (1), users expect TS to take those paths mappings and convert them to relative paths so they will work cause they are not in node_modules....

With (2) user's will not expect that to happen because it will cause the imports to point to nothing.

I understand TS's team take on this, it is something that will work with (1) but not (2) and that's enough to rule it out.

Maybe i'm wrong, but it seems to me it's one of the reasons for the mixup

Optional flag = Developer will have the option to enable it for #1 of your case; which is the majority of the cases as can be seen by the number of upvotes here.

winseros commented 4 years ago

If path maps are not resolved into emitted code, then what are they for?

DanielRosenwasser commented 4 years ago

@winseros this heavily downvoted comment I wrote a few months ago answers that question. https://github.com/microsoft/TypeScript/issues/26722#issuecomment-516935532

RyanCavanaugh commented 4 years ago

We took this to several design meetings and discussed this to death, and we're sticking with the "JS is JS" design philosophy that has driven TypeScript since its origination.

I'm going to lock this because it's a long thread and our typical experience is that these threads go in circles when it takes more than a few minutes to read the thread. As usual, I'd implore anyone to actually read all 81 comments here, as we ourselves have done multiple times, before filing new issues related to the matter.