Proposal to make ts-loader a better webpack citizen

MortenHoustonLudvigsen commented 7 years ago

Working on pull request #520, and reading the comments in #506, has made me realise that ts-loader is not a well behaved webpack loader.

From the webpack documentation for how to write a loader:

Guidelines

(Ordered by priority, first one should get the highest priority)

Loaders should do only a single task

Loaders can be chained. Create loaders for every step, instead of a loader that does everything at once.

This also means they should not convert to JavaScript if not necessary.

Loaders should do only a single task

ts-loader does only do a single task: compiling TypeScript modules.

Loaders should not convert to JavaScript if not necessary

ts-loader does convert to JavaScript, and it is necessary.

Loaders can be chained

This is where ts-loader is not a well behaved webpack loader.

The TypeScript compiler needs to have access to modules imported into the current module. ts-loader does this by reading directly from the file system, if the file has not been seen previously. It is therefore not always possible to make a previous loader in the same chain work properly (even with pull request #520).

Proposal

Goals

To make ts-loader a better webpack citizen I propose that the ts-loader is changed to achieve the following:

TypeScript files should be resolved as webpack modules, using whatever resolution rules are set up for webpack.
ts-loader should never read modules directly from the file system

The method loaderContext.loadModule, or similar, should be used in stead. This will enable other loaders to make changes to source files, or even generate virtual source files.
ts-loader should never read a module more than once during a compilation

This is essential for good performance, and should avoid problems with circular references.
During watch ts-loader should only read a module if it has changed

This is essential for good performance. It should be investigated whether ts-loader needs to track changed files to achieve this, or whether webpack can do so.

Implementation

Because loading modules in webpack is an asynchronous operation, and the TypeScript compiler reads files synchronously, it will probably be necessary to compile each file in several phases:

Save the actual TypeScript source code (as it is after running through previous loaders) in a module cache.
Get a list of all imported modules using ts.preProcessFile.
For each imported module, that is not already in the module cache, do the following (these are asynchonous operations):
1. Each possible path for the module should be resolved by trying to add .ts, .tsx, .d.ts, etc. and using loaderContext.resolve.
2. Try loading each of the resolved paths until loading succeeds. Save the succeeded module source code in the module cache, or mark the module as non existent. See if we can load any TypeScript modules without compiling them (for example by adding a query parameter to the module request).
  This could be achieved using loaderContext.loadModule. However, loaderContext.loadModule does not load modules recursively, so it may be necessary to code our own loadModule function (as I have done in ts-css-loader).
Synchronously compile the TypeScript source using source code saved in the module cache.

Conclusion

I would be very happy to work on this, but only if there is interest.

johnnyreilly commented 7 years ago

There's interest. :+1: :smile:

I'm assuming that the existing test packs would guard against any regressions and so in my view we have nothing to lose and potentially much to gain. Godspeed John Glenn!

MortenHoustonLudvigsen commented 7 years ago

I'm glad 😄

I will create a long running Pull Request within the next few days, so you can follow the progress. However, I'm not sure I will have time to dive deeply into this before August.

I am also thinking of introducing unit tests for the new code, if you have no objections.

MortenHoustonLudvigsen commented 7 years ago

I have created branch a-better-webpack-citizen in my fork of ts-loader for this. As soon as I have made any changes I will create the PR.

jbrantly commented 7 years ago

First off, realize that I'm coming from a place of ignorance here in that I haven't really looked at the ts-loader code in detail since the great refactoring and I haven't exactly been following along with recent developments either.

However, at least at one point in the past, ts-loader could be chained properly with a performance penalty. As you've said, TypeScript loads all files from disk currently. The "trick" was to compare what TypeScript thought a file looked like (using TypeScript's internal cache) and what webpack thought a file looked like (using the incoming source for the module) and update TypeScript's internal cache if they differed. It was a pretty simple solution and it worked.

I'm not familiar with loaderContext.loadModule but it sounds like that could be useful in this situation. I think the catch is that it's asynchronous and TypeScript's readFile is synchronous as you've pointed out. Due to this I'm not sure how much your proposal adds over the simple "check if current module is outdated" mechanism described above.

Regarding module resolution rules, I'm not sure. I personally believe that the original goal ts-loader to be a "drop-in replacement" of tsc is still important, which means that the TypeScript resolution rules are important. Ideally you could create resolution rules in tsconfig (which means that your app is still tsc compilable and various IDEs will also just work) and ts-loader would be able to resolve using those rules as well. So when you say "using whatever resolution rules are set up for webpack" that gives me a little hesitation.

jbrantly commented 7 years ago

Also, is it just me or would things work a whole lot better if TypeScript's operation was asynchronous? Does anyone know if this has been brought up to them before?

johnnyreilly commented 7 years ago

@MortenHoustonLudvigsen

However, I'm not sure I will have time to dive deeply into this before August.

Whenever you get the time - all contributions are appreciated!

I am also thinking of introducing unit tests for the new code, if you have no objections.

None at all!

@jbrantly Thanks so much for pitching in - your insights are always very valuable 👍

First off, realize that I'm coming from a place of ignorance here in that I haven't really looked at the ts-loader code in detail since the great refactoring and I haven't exactly been following along with recent developments either.

I think you'd be surprised as to how similar the code is; essentially the main part of the refactoring was making the code more modular. The underlying logic isn't too different.

Regarding module resolution rules, I'm not sure. I personally believe that the original goal ts-loader to be a "drop-in replacement" of tsc is still important, which means that the TypeScript resolution rules are important. Ideally you could create resolution rules in tsconfig (which means that your app is still tsc compilable and various IDEs will also just work) and ts-loader would be able to resolve using those rules as well. So when you say "using whatever resolution rules are set up for webpack" that gives me a little hesitation.

The points here I completely agree with. The experience of using ts-loader and tsc should be identical as near as possible. The situation I fear is your IDE / code editor giving you different information about your code to ts-loader. If that ever happens then it's a big, big issue.

Also, is it just me or would things work a whole lot better if TypeScript's operation was asynchronous? Does anyone know if this has been brought up to them before?

I have no idea if it's been brought up before - do feel free to do just that!

blakeembrey commented 7 years ago

I believe there's an issue in the past on async if you can find it, and it's a pretty massive change to the TypeScript codebase so I can understand pushback on it. However, it would be possible to hack around this for now - there's a method exposed by TypeScript for parsing a text file and extracting all the module imports quickly (see ts.preProcessFile(contents) - https://github.com/typings/core/blob/119e979b73c438a2f0e6a1c7c24faa57ebae7eeb/src/lib/compile.ts#L369). Just use that, pass all files to loaderContext.loadModule (never used this myself, but hopefully it takes relative/module reference strings), put loaded files in a cache and run TypeScript afterwards. Have the read file method for TypeScript reading from the cache instead of the filesystem and should be able to avoid ever hitting the filesystem from this module.

johnnyreilly commented 7 years ago

Thanks @blakeembrey that's super helpful!

MortenHoustonLudvigsen commented 7 years ago

@jbrantly

Thank you for your comments. I really don't want to go down the wrong road with this.

However, at least at one point in the past, ts-loader could be chained properly with a performance penalty. As you've said, TypeScript loads all files from disk currently. The "trick" was to compare what TypeScript thought a file looked like (using TypeScript's internal cache) and what webpack thought a file looked like (using the incoming source for the module) and update TypeScript's internal cache if they differed. It was a pretty simple solution and it worked.

This is, as far as I can see, still the case. However this only happens to the file being loaded - not to any files it imports - directly or indirectly. This means that any changes to those files will not be available until they themselves have been through the loader. So if another loader should change or add types in an imported module, that has not been loaded yet, the type checking will not be correct.

I'm not familiar with loaderContext.loadModule but it sounds like that could be useful in this situation. I think the catch is that it's asynchronous and TypeScript's readFile is synchronous as you've pointed out. Due to this I'm not sure how much your proposal adds over the simple "check if current module is outdated" mechanism described above.

As @blakeembrey writes, ts.preProcessFile can be used to discover any files a TypeScript module depends on, and can be used to preload the files. This is, as far as I have been able to ascertain, a very fast function.

Regarding module resolution rules, I'm not sure. I personally believe that the original goal ts-loader to be a "drop-in replacement" of tsc is still important, which means that the TypeScript resolution rules are important. Ideally you could create resolution rules in tsconfig (which means that your app is still tsc compilable and various IDEs will also just work) and ts-loader would be able to resolve using those rules as well. So when you say "using whatever resolution rules are set up for webpack" that gives me a little hesitation.

I see your point, and agree that this should be the default. However, if I make the resolution pluggable, we could easily introduce an option for webpack resolution in the future.

Also, is it just me or would things work a whole lot better if TypeScript's operation was asynchronous? Does anyone know if this has been brought up to them before?

I totally agree, and it has been discussed here: Microsoft/TypeScript#1857. It does not sound like this will change any time soon.

@blakeembrey

Thank you for your comment. This is exactly how I envision implementing this.

etaque commented 7 years ago

Hello, could that (loaders behaviour) be the reason why we can't import Elm modules directly from TS? Example here: https://github.com/etaque/webpack-elm-from-ts-error It works if I reexport directly the Elm module from a JS file, then include JS from TS.

andrewbranch commented 6 years ago

I recently discovered the same issue, after having written a series of loaders that transform a Markdown file into a valid TypeScript file exporting a React component. Needless to say, it didn’t work at all—ts-loader was attempting to compile the original raw markdown file as TypeScript.

Is there any interest in this still? I was excited to stumble upon this issue, and then dismayed to see that the a-better-webpack-citizen branch has no commits ahead of master 😦

johnnyreilly commented 5 years ago

Yeah I think there's still interest!

danon commented 3 years ago

Is this feature still being developed?

johnnyreilly commented 3 years ago

Don't think so

elad-yosifon commented 1 month ago

@johnnyreilly I'm working on a plugin that inlines const enums across the entire project (before ts-loader runs).

In transpileOnly=true mode, everything works as expected - ts-loader tries to compile the provided source contents.

In transpileOnly=false mode, typescript compiler is not pleased with the transformed code not matching the filesystem code (my plugin removes exported const enums after inlining them, thus breaking source code compilation).

Just to be extra clear, this is how my plugin is set-up:

Webpack build -> 
  run plugins:
    [MyPlugin] (working on TS artifacts in memory), 
    [...all other plugins] (probably working on JS artifacts after ts-loader emits)
  run loaders:
    [myPluginLoader] (loads preprocessed TS artifacts from memory, and feeds it to subsequent loaders),
    [ts-loader] (compiles TS artifacts)

If ts-loader will feed typescript compiler the in-flight code, instead of reading from filesystem, that will solve it. This will allow me to publish my plugin, and do a lot of good to the TS community 😄 (and a greener planet 🌎🍃).

johnnyreilly commented 1 month ago

Given the amount of time that has elapsed, and that no complete implementation has been provided that contributes this, it seems unlikely that this feature is going to ship I'm afraid.

elad-yosifon commented 1 month ago

@johnnyreilly I am willing to chip in, can you provide some guidance and point me in the right direction?

johnnyreilly commented 1 month ago

I appreciate the offer @elad-yosifon but I'm time poor right now. Sorry

danon commented 1 month ago

@elad-yosifon I empathize with you :/ ts-loader is a de-facto standard for webpack and typescript, and its author is time-poor, even in light of willing contributors.

johnnyreilly commented 1 month ago

If you can put together a PR I'll certainly look at it - but I'm sorry I don't have the time to get you moving myself. If someone else can, please feel free. I'm afraid I am involved in a number of things and I'm trying to be wise about how I split my time. There's only so many things I can do

elad-yosifon commented 1 month ago

@johnnyreilly I can totally relate. I’ll try to hack my way into it.

elad-yosifon commented 1 week ago

@johnnyreilly is #1450 a solution for this issue?

johnnyreilly commented 1 week ago

I don't think so - but it's been a while and I can't remember the details

TypeStrong / ts-loader