TypeStrong / ts-loader

TypeScript loader for webpack
https://johnnyreilly.com/ts-loader-goes-webpack-5
MIT License
3.46k stars 431 forks source link

Support for TS 3 project references #815

Closed VojtaStanek closed 6 years ago

VojtaStanek commented 6 years ago

We have a monorepo when one of the packages is compiled with webpack, but references other package from the monorepo. We are using project references (new in TS 3) to compile only the packages, that are needed (the changed one and other that have reference to it). Is there a way to make it work with webpack? Currently we are compiling it with tsc and then bundling compiled js with webpack, but it is not an ideal solution, since many features doesn't work so well (e.g. source maps and custom loaders such as sass-loader) and it is quite slow.

johnnyreilly commented 6 years ago

Hey @cactucs,

Project references support isn't implemented as yet. If you'd like to we'd be happy to look at a PR. If you're wondering what would need to change then take a look at the gulp-typescript changes as a guide:

https://github.com/ivogabe/gulp-typescript/pull/579/files

Don't worry about broken tests; feel free to just start the PR process and we can use that as a basis for collaboration :smile:

andrewbranch commented 6 years ago

@johnnyreilly I started taking a look at this; I got curious about it at the exact same time as @Cactucs. It seems that ts-loader’s transpileOnly path is pretty similar to what gulp-typescript does, but the code paths that use the language service and the experimental watch API are pretty different.

As a test, I wrote a project where the root tsconfig file uses strict: false and the referenced project tsconfig uses strict: true, and contains some code that has a semantic error due to strict mode. I did figure out how to get getProjectReferences onto the typescript.LangaugeServiceHost, but the compiler doesn’t come up with the error in languageService.getSemanticDiagnostics(). I stepped into the compiler source itself and verified that the program instance does indeed have program.getProjectReferences() that seems to be working correctly.

I'll fork and push what I've done so far.

andrewbranch commented 6 years ago

Here's the diff: https://github.com/TypeStrong/ts-loader/compare/master...andrewbranch:project-references?expand=1

andrewbranch commented 6 years ago

That said, I'm not sure what exactly the expected behavior for webpack and project references is. tsc --build works in part by checking timestamps on the emitted JS and d.ts files from dependent composite projects, but of course using webpack is going to totally change what happens to that emit (gets piped into the bundler instead of written to disk right away). At least, semantic error checking should work against any composite projects’ specific tsconfig, but otherwise, how do project references change what this loader is supposed to do?

johnnyreilly commented 6 years ago

First of all, thanks for your work @andrewbranch!

That said, I'm not sure what exactly the expected behavior for webpack and project references is

An excellent question; and honestly I'm not sure I know the answer.

I'll cc @RyanCavanaugh @rbuckton in case they have a view. But for myself I'm not too sure.

That said, I'm open to exploring options.

andrewbranch commented 6 years ago

Just pushed a commit that “fixes” making the language service aware of project references, causing an error to occur when the dependent composite project isn’t built (which is indeed what tsc does without --build), so that seems like a step forward. Need to update the error reporting hooks so that the error that’s shown isn’t

Error: Could not find file: '/Users/Andrew/Developer/webpack-dependent-projects-test/src/shared/index.ts'.
    at getValidSourceFile (/Users/Andrew/Developer/ts-loader/node_modules/typescript/lib/typescript.js:111577:23)
    at Object.getSyntacticDiagnostics (/Users/Andrew/Developer/ts-loader/node_modules/typescript/lib/typescript.js:111765:52)
    at provideErrorsToWebpack (/Users/Andrew/Developer/ts-loader/dist/after-compile.js:113:36)
    at /Users/Andrew/Developer/ts-loader/dist/after-compile.js:23:9

but rather the semantic diagnostic generated by the program, which you see with tsc:

error TS6305: Output file '/Users/Andrew/Developer/webpack-dependent-projects-test/dist/index.d.ts' has not been built from source file '/Users/Andrew/Developer/webpack-dependent-projects-test/src/shared/index.ts'.
src/index.ts(1,25): error TS2307: Cannot find module './shared'.

As I'm beginning to understand more how this works, it seems like ts-loader could take one (or either/both with a loader option) of two positions:

  1. ts-loader just works like running tsc. Existing built composite projects will be used instead of recompiling the source files, which does have the advantage of being faster, not including the time spent outside of webpack building those composite projects. I think getting this far is pretty easy—similar to what gulp-typescript added in the PR you linked.
  2. ts-loader works like running tsc --build, rebuilding composite projects when files inside those projects change. I have no idea how difficult that would be; I skimmed through some TypeScript APIs and couldn’t find if or where that orchestrator functionality is exposed. I think this would be useful, though, as it would eliminate the need for adding more complexity to our existing development pipeline (I guess we’d have to spin up a separate watch task to watch the dependent projects if we wanted to keep our current behavior of getting a rebuild when changing any referenced file, even ones that we don’t change that often, which we’d like to put in a composite project.
johnnyreilly commented 6 years ago

it seems like ts-loader could take one (or either/both with a loader option) of two positions

Please keep investigating. I'm open to having an option that flips between modes if that seems useful. Probably best you do some real world experiments and report back on what seems to make sense.

andrewbranch commented 6 years ago

Sounds good to me; I'm happy to keep experimenting, but would definitely appreciate input from the TS team here. Technically I guess they’re coworkers of mine so maybe I can track someone down. 😄

Pushed another commit that basically finishes option 1 for the language service path; should hopefully be simple enough to do the same for the createProgram and createWatchProgram paths.

I've been testing and debugging ad-hoc for this so far; the test directory here is daunting. Any tips on where to start if I were to begin writing some real tests?

Edit: also, do you have a preference on how to do TypeScript version detection? We can only call program.getProjectReferences() on >= 3.0.

johnnyreilly commented 6 years ago

Sounds good to me; I'm happy to keep experimenting, but would definitely appreciate input from the TS team here.

Great! @mhegazy @DanielRosenwasser @sheetalkamat @RyanCavanaugh @rbuckton would any one of you be able to advise your erstwhile colleague @andrewbranch on what a sensible course of action here would look like?

I've been testing and debugging ad-hoc for this so far; the test directory here is daunting. Any tips on where to start if I were to begin writing some real tests?

Yeah it is a bit. Essentially there's 2 test suites; each with a subtly different purpose. The comparison test pack and the execution test pack. Have a read of the README's to find out more about them:

https://github.com/TypeStrong/ts-loader/blob/master/test/comparison-tests/README.md https://github.com/TypeStrong/ts-loader/blob/master/test/execution-tests/README.md

The TL;DR is this: the comparison test pack captures file outputs for future diffing. It's useful for checking files are generated in response to various actions (you have fake files being updated and capture the resulting output). The execution test pack actually executes tests written in TypeScript, and transpiled into JS. These tests are more reliable but don't provide any insight as to what triggers file generation.

Probably given your use case a comparison test will be most appropriate. But I know these are daunting and I'm happy to pitch in and help guide you.

Edit: also, do you have a preference on how to do TypeScript version detection? We can only call program.getProjectReferences() on >= 3.0.

Probably just doing program.getProjectReferences && program.getProjectReferences() would work? I don't feel super strongly. Aim for readability and simplicity but otherwise go nuts :smile:

cc @thelarkinn

andrewbranch commented 6 years ago

@johnnyreilly, how would you feel about splitting my proposed two possibilities into two PRs? I think I’m almost done with the first part (using built project references, but not building them), which I think is useful on its own, and I would hate to see it languish if figuring out the second part takes a long time or I can’t commit as much time in the future. I'll go ahead and open a WIP PR and start trying to figure out the tests.

andrewbranch commented 6 years ago

I’ve hit a bit of a speed bump upon realizing that program.getSourceFile() for a file in a project reference, assuming the dependent project is built, returns a SourceFile object describing the .d.ts declaration for that file/project. For Webpack’s purposes though, we need to get the corresponding .js file, and I’m having trouble figuring out an API to do that. Although, I think it’s guaranteed that the JS file will be colocated and share the same name as the declaration file... but that feels a bit sketchy to me...?

johnnyreilly commented 6 years ago

I think it’s guaranteed that the JS file will be colocated and share the same name as the declaration file... but that feels a bit sketchy to me...?

This is true unless they've set declarationDir in their tsconfig.json I believe; see https://github.com/TypeStrong/ts-loader/blob/master/src/tsconfig.json for an example.

So you should be able to derive the location of the original source file using the info in the tsconfig.json. it seems reasonable enough to try and use that as the basis for a decision?

RyanCavanaugh commented 6 years ago

Hey all, thanks for taking this up. Here to answer some questions (I'll be checking in on this all day so let's get chatty).

To compute the output JS filename, you can use this code from src/compiler/tsbuild.ts:

    function getOutputJavaScriptFileName(inputFileName: string, configFile: ParsedCommandLine) {
        const relativePath = getRelativePathFromDirectory(rootDirOfOptions(configFile.options, configFile.options.configFilePath!), inputFileName, /*ignoreCase*/ true);
        const outputPath = resolvePath(configFile.options.outDir || getDirectoryPath(configFile.options.configFilePath!), relativePath);
        const newExtension = fileExtensionIs(inputFileName, Extension.Json) ? Extension.Json :
                             fileExtensionIs(inputFileName, Extension.Tsx) && configFile.options.jsx === JsxEmit.Preserve ? Extension.Jsx : Extension.Js;
        return changeExtension(outputPath, newExtension);
    }

Some general discussion - in a way, project references and tsbuild's behavior of building upstream projects are actually totally orthogonal, though obviously related. Project references are just an advanced form of path mapping, and tsbuild's build ordering/dependency algorithm is selected mostly for simplicity.

The choice of building referenced projects or not, and the choice of what the right up-to-date check, really depends on the tool and I'm honestly not well-informed enough about Webpack to make the right call on whether there's one right answer or space for configurability here.

andrewbranch commented 6 years ago

@RyanCavanaugh awesome, thanks! Appreciate you taking the time to offer some expertise here.

One immediate question: looking at the snippet from tsbuild.ts you pasted, that function isn’t exported and most of the functions it calls are also not exported—would the team consider exporting this so we don’t have to duplicate the logic?

RyanCavanaugh commented 6 years ago

@andrewbranch certainly. If you can come up with a list we can open a bug on our side and review them

andrewbranch commented 6 years ago

I think getOutputJavaScriptFileName is the only thing I need for now, as I think the only thing left to do for getting ts-loader to understand project references is to be able to find the JS for a given file inside a referenced project.

@johnnyreilly this is making me realize, though... if the referenced project uses outFile to produce a single concatenated output JS file, we probably don’t want to read that whole file’s contents as the loader output, because then Webpack will have one copy of that whole outFile for every source file in the referenced project. My loader API knowledge isn't deep enough to know if there's an easy solution for this—intuitively it seems we’d want to "redirect" the loader to another file, or somehow let Webpack know that multiple input files actually become the same output, but I'm not sure that's possible. Thoughts?

EDIT: alternatively, we could choose not to support outFile in project references, at least while ts-loader isn’t going to build upstream projects itself. If the ultimate goal is to build with Webpack, I think the user likely won’t care whether or not they can use outFile 🤔

johnnyreilly commented 6 years ago

@johnnyreilly this is making me realize, though... if the referenced project uses outFile to produce a single concatenated output JS file, we probably don’t want to read that whole file’s contents as the loader output

Yeah - I don't think this works in the scenario with outDir. But then I think that's probably to be expected; it seems like a reasonable choice.

ts-loader generally tries to be a drop-in replacement for tsc except where it doesn't make sense. This is that!

andrewbranch commented 6 years ago

@RyanCavanaugh the only other thing that's a little awkward at the moment that maybe you can help with: determining the project reference (if one exists) for a given file path. Right now I'm doing

program.getProjectReferences().find(ref => ref.commandLine.fileNames.includes(fileName))

Is there a more efficient way, whether public or internal, for getting that?

EDIT: aside from getting the JS output file, we’ll also need to get the source map output if it exists, although that one I assume is always just swapping the extension on the JS output filename—right?

RyanCavanaugh commented 6 years ago

@andrewbranch I believe that's the simplest way to do it, yes. I don't think we provide any API for it

that one I assume is always just swapping the extension on the JS output filename—right?

Correct; there are currently no compiler options which move the .map file

pkieltyka commented 6 years ago

It seems the presence of the "composite": true setting in tsconfig.json makes ts-loader v4.5.0 blow up.

I'm using "composite": true in one package, so my ts-node server package that uses tsc to build can use that referenced package's files.. however, im getting the following error when using my webpack config + ts-loader (the only difference from it working or not, is composite true/false flipped):

 94% after sealType checking in progress...
✖ 「wdm」:    49 modules

ERROR in ./src/index.tsx
Module build failed (from ../node_modules/ts-loader/index.js):
TypeError: Cannot read property 'replace' of undefined
    at Object.normalizeSlashes (/Users/peter/Dev/go/src/github.com/horizon-games/SkyWeaver-gg/game/node_modules/typescript/lib/typescript.js:14080:21)
    at getCommonSourceDirectory (/Users/peter/Dev/go/src/github.com/horizon-games/SkyWeaver-gg/game/node_modules/typescript/lib/typescript.js:82737:68)
    at verifyCompilerOptions (/Users/peter/Dev/go/src/github.com/horizon-games/SkyWeaver-gg/game/node_modules/typescript/lib/typescript.js:84278:27)
    at Object.createProgram (/Users/peter/Dev/go/src/github.com/horizon-games/SkyWeaver-gg/game/node_modules/typescript/lib/typescript.js:82703:9)
    at Object.transpileModule (/Users/peter/Dev/go/src/github.com/horizon-games/SkyWeaver-gg/game/node_modules/typescript/lib/typescript.js:100518:26)
    at getTranspilationEmit (/Users/peter/Dev/go/src/github.com/horizon-games/SkyWeaver-gg/game/node_modules/ts-loader/dist/index.js:231:74)
    at successLoader (/Users/peter/Dev/go/src/github.com/horizon-games/SkyWeaver-gg/game/node_modules/ts-loader/dist/index.js:33:11)
    at Object.loader (/Users/peter/Dev/go/src/github.com/horizon-games/SkyWeaver-gg/game/node_modules/ts-loader/dist/index.js:21:12)
ℹ 「wdm」: Failed to compile.
simonbuchan commented 6 years ago

@pkieltyka You've probably found this already, but just in case: you're hitting Microsoft/TypeScript#26554, which you can work around without breaking tsc by adding options: { compilerOptions: { composite: false } } to your loader config.

pkieltyka commented 6 years ago

thank you @simonbuchan — thats a nice trick to know. I ended up making a separate tsconfig.ref.json file which I extended from a version without composite: true and used ts-loader, but, knowing this trick it lets me remove that file, nice.

Will ts-loader be able to support project references soon? this would be helpful in a monorepo situation, something we're doing in our setup already with ts-node on a server project (and works very well)

andrewbranch commented 6 years ago

@pkieltyka I'm hoping to get #817 merged next week. Note that means you'll still need to run tsc --build yourself somehow, but then ts-loader will take advantage of the built output from that. Thanks for your patience! Was hoping it would have been done by now myself. :)

pkieltyka commented 6 years ago

@andrewbranch thats awesome, perhaps trigger tsc -- build along the lines as done like this plugin: https://github.com/Realytics/fork-ts-checker-webpack-plugin — you could make a fork-ts-project-references-build-webpack-plugin (lol, find shorter name), or offer the additional plugin directly inside of ts-loader to be hooked into the build cycle

johnnyreilly commented 6 years ago

FWIW I'm a maintainer of https://github.com/Realytics/fork-ts-checker-webpack-plugin also and I'd be happy to review PRs to that as well 😊

mscottnelson commented 6 years ago

I'm having an issue with this option enabled, where it appears ts-loader attempts to load the references even though I have projectReferences set to true.

My structure:

project
└───primaryProject (builds to dist)
│   │   tsconfig.json
│   │   webpack.common.js
│   └───src
│   └───dist
└───sharedLib (builds in place)
│   └───tsconfig.json
└───sharedLib2 (used by sharedLib and primaryProject, builds in place)
    └───tsconfig.json

I attempt to build via an npm script ie tsc -b --verbose && webpack --progress --config webpack.dev.js

Anybody, especially @andrewbranch , have an idea why this might be causing me problems with this new projectReferences feature? It seems to me that ts-loader would ignore the ts in the sharedLibs and use the (now built and up-to-date) js files in that same location instead.

Edit: Additional detail of error:

ERROR in ../sharedLib/src/printHelloNode.ts
Module build failed (from ./node_modules/ts-loader/index.js):
Error: TypeScript emitted no output for /[project]/sharedLib/src/printHelloNode.ts.
    at makeSourceMapAndFinish (/[project]/primaryProject/node_modules/ts-loader/dist/index.js:78:15)
    at successLoader (/[project]/primaryProject/node_modules/ts-loader/dist/index.js:68:9)
    at Object.loader (/[project]/primaryProject//node_modules/ts-loader/dist/index.js:22:12)
 @ ./src/helloExample.ts 4:25-74
 @ multi ./src/helloExample.ts

ERROR in ../[project]/sharedLib2/src/printHelloCommons.ts
Module build failed (from ./node_modules/ts-loader/index.js):
Error: TypeScript emitted no output for /[project]/sharedLib2/src/printHelloCommons.ts.
    at makeSourceMapAndFinish (/[project]/primaryProject/node_modules/ts-loader/dist/index.js:78:15)
    at successLoader (/[project]/primaryProject/node_modules/ts-loader/dist/index.js:68:9)
    at Object.loader (/[project]/primaryProject/node_modules/ts-loader/dist/index.js:22:12)
 @ ./src/helloExample.ts 3:28-84
 @ multi ./src/helloExample.ts
andrewbranch commented 6 years ago

Hey @mscottnelson, would you be able to link to your project or an example that reproduces the issue? I'd have to take a look at what's in each tsconfig.

mscottnelson commented 6 years ago

Hi @andrewbranch , many thanks for the rapid response. I suspect this is happening because the path references are not migrated / updated to point to the new relative locations when Webpack produces its output directory/file. Not sure what the most idiomatically correct solution would be.

Here's an example that reproduces the issue: https://github.com/mscottnelson/projectRefTsLoaderExample

andrewbranch commented 6 years ago

Hey @mscottnelson! Just did some debugging, and figured out that you are hitting the exact same thing that I hit here: https://github.com/TypeStrong/ts-loader/pull/817#discussion_r210029934.

Specifically, when I follow Webpack’s request for ../isomorphicLib/src/printHelloCommons.ts, we look through the list of resolved project references, and we ask each one, “do you have a file called ../isomorphicLib/src/printHelloCommons.ts”? And each one says “nope, I don’t have any files at all!” More scientifically, each resolvedProjectReference.commandLine.fileNames is an empty array.

The weird thing is, though, that tsc --build (and afterwards, just tsc) works totally fine, and the files in that project obviously get built. It’s like tsc --build is helping you out by assuming a default set of fileNames, but isn’t making that information available programmatically. I wonder if that could have changed between TS 3.0.1 and 3.0.3.

At the moment, if you add a files or include to each composite tsconfig, you should be unblocked. But it does feel like pretty much everyone is going to hit this, so I need to investigate more what’s going on. At the very least, we can throw a helpful warning if we see an empty array of fileNames, but since tsc works, my feeling is that ts-loader should work too.

By the way, for anyone interested in looking into this, or other ts-loader issues, I quickly debugged with

node --inspect-brk node_modules/webpack/bin/webpack.js --config webpack.dev.js

then put a breakpoint in node_modules/ts-loader/dist/index.ts, then ran “Attach to Node Process” in VS Code. The dist is JS compiled from TS, but it’s still pretty readable.

sheetalkamat commented 6 years ago

resolvedProjectReference.commandLine.fileNames is an empty array

Could it be because your host doesn't have readDirectory implemented correctly?

andrewbranch commented 6 years ago

@sheetalkamat hmm, my PR didn't change that, but you would certainly know better than I! 😄 It looks pretty innocuous here: https://github.com/TypeStrong/ts-loader/blob/f945f4f949a152113e373cde9970a260aebdc369/src/servicesHost.ts#L124

Would you expect something different?

andrewbranch commented 6 years ago

Ok, so if I check the resolved project reference object more closely, it has an error in commandLine.errors with code 18003 and message:

No inputs were found in config file 'isomorphicLib/tsconfig.json'. Specified 'include' paths were '["**/*"]' and 'exclude' paths were '[]'."

So there is a default include, but for some reason it’s not working. In fact, if I manually add an include to the tsconfig.json, it doesn’t matter what the include is—it never matches anything 😦

So in fact, when I said

At the moment, if you add a files or include to each composite tsconfig, you should be unblocked.

It looks like I was wrong. Maybe only files works at the moment. Let me see what I can figure out here.

andrewbranch commented 6 years ago

Could it be because your host doesn't have readDirectory implemented correctly?

@sheetalkamat you were exactly right! Well, almost—I don't know if I would call it my host—it looks like TypeScript was creating one for me, and it didn't put readDirectory in there. It was fixed 24 days ago: https://github.com/Microsoft/TypeScript/commit/d519e3f21ec36274726c44dab25c9eb48e34953f by @ajafff, whose intersections with my work here have been incredibly helpful multiple times now. 😄

And @mscottnelson, sure enough, if you upgrade your example project to typescript@next, it works like a charm 🎉

johnnyreilly commented 6 years ago

Boom! Does this mean that project references should start to work as expected when 3.1 ships?

I'm assuming this will ship with 3.1:

https://github.com/Microsoft/TypeScript/commit/d519e3f21ec36274726c44dab25c9eb48e34953f

andrewbranch commented 6 years ago

@johnnyreilly the tag says 3.1.1, but seeing as 3.1 is currently in RC, I think it's all one and the same? I'm opening a PR now to add a note to the README. Do you want to add a note to your blog post?

johnnyreilly commented 6 years ago

Good shout - just waking up over here. Will try and do that later today. Good investigation!

BTW I might take your debugging tips and document them somewhere; I've always relied on logging to diagnose issues with ts-loader thus far. Being able to use Code to properly debug is :100: times better!

My kingdom for a debugger :smile:

simonbuchan commented 6 years ago

I've had break points in original source working before, though I use Webstorm. You could try ndb?

johnnyreilly commented 6 years ago

You could try ndb?

I might take it for a whirl at some point. I use both Chrome and Code to debug in other contexts and find both tremendous.

mscottnelson commented 6 years ago

Many thanks for the help. Updating to TypeScript 3.1.1 with options: { projectReferences: true } and running tsc -b before webpack now works.

andrewbranch commented 6 years ago

Small status update: I spent an hour or so this morning exploring the current state of the tsbuild API to see if it contains what we need for ts-loader to build/watch upstream projects. It mostly does, but currently the API isn't finalized and the whole thing is marked internal: https://github.com/Microsoft/TypeScript/blob/85a3475df87b9c10240570f32090970262c99b54/src/compiler/tsbuild.ts#L1-L2

I think ts-loader’s full support for project references will have to wait until that’s finalized, lest we get broken by minor or patch TypeScript releases.

@sheetalkamat, two things:

  1. Would you be able to share with us whether that API is imminent, or more of a longer term goal?
  2. The thing that ts-loader needs to be able to do, eventually, is build every dependency of a project, but not that project itself. After a little exploring this morning, it looks like the way to do that (in the hypothetical world where today’s internal APIs are final) is currently something like:
const project = path.resolve('./tsconfig.json'); // “Root” project, has dependencies
const builder = ts.createSolutionBuilder(host, [project], {});
const { buildQueue } = builder.getBuildGraph([project]);

// buildQueue is an array of paths to tsconfig files, in the order that they need to be built.
// That means our “root” project is the last element in the array. We want to build every
// project _except_ that one.
const projectsIWantToBuild = buildQueue.slice(0, buildQueue.length - 1);
const dependencyBuilder = ts.createSolutionBuilder(host, projectsIWantToBuild, {});
dependencyBuilder.buildAllProjects();

I realize all this is still in flux, so I just wanted to give you an idea of the functionality we’ll need here, since it’s not the typical case of running a solution build programmatically. At the moment, it feels a little kludgy to have to create two SolutionBuilders. Might there be a way in the future to build all projects with a filter, or simply to build all dependencies of a project? Thanks for all your work on this!

johnnyreilly commented 6 years ago

Thanks for continuing to look into this @andrewbranch. Given that this is a closed issue and I'm vaguely aware that not everyone tracks closed issues, I thought I'd open up a new issue specifically related to ts-loader project references build support. Aid visibility and all that. I've copied your comment above there.

ANYONE READING THIS INTERESTED IN LEARNING ABOUT ADDING BUILD SUPPORT TO TS-LOADER PLEASE LOOK AT: https://github.com/TypeStrong/ts-loader/issues/851