New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support project references #817

Merged
merged 44 commits into from Sep 23, 2018

Conversation

Projects
None yet
4 participants
@andrewbranch
Copy link
Contributor

andrewbranch commented Aug 10, 2018

Closes #815

TODO:

  • support project references for transpileOnly
  • support project references for experimentalWatchApi
  • support project references for running with neither of the above
  • resolve loader output to built JS files for TS files that were part of a project reference
  • tests (for each of the three modes above, one test for a successful build and one that fails because the dependent project isn’t built)
  • get Webpack to watch the built JS files instead of TS files in composite projects
  • manually test on a real-life big project
  • write execution tests
@johnnyreilly

This comment has been minimized.

Copy link
Member

johnnyreilly commented Aug 10, 2018

This is PR "1" of the potential 2 I guess? Great work so far! 👍

You've just reminded me that I need to update the comparison test pack for TS 3.0. I'll try and do that this weekend so you don't have to.

I'll message here when it's done.

@johnnyreilly

This comment has been minimized.

Copy link
Member

johnnyreilly commented Aug 11, 2018

I've upgraded the comparison test pack to 3.0 - feel free to merge that on to your pr

@andrewbranch andrewbranch force-pushed the andrewbranch:project-references branch from dadf210 to 2b8e530 Aug 13, 2018

@andrewbranch

This comment has been minimized.

Copy link
Contributor

andrewbranch commented Aug 13, 2018

Rebased against master with the updated tests 👍

Latest commit will fail; I'm going ahead and assuming we'll have typescript.getOutputJavaScriptFile via Microsoft/TypeScript#26410.


if (!instance.compiler.sys.fileExists(mapFileName)) {
loader.emitWarning(

This comment has been minimized.

@andrewbranch

andrewbranch Aug 13, 2018

Contributor

It might be obnoxious to do this for every file. Maybe I can memoize by referencedProject.sourceFile.fileName? Or would it make more sense to validate each project reference while first setting up the program/languageService/watch?

This comment has been minimized.

@johnnyreilly

johnnyreilly Aug 13, 2018

Member

That seems reasonable; presumably that's not something that changes often at all so should be a completely reasonable choice

Show resolved Hide resolved src/utils.ts
@andrewbranch

This comment has been minimized.

Copy link
Contributor

andrewbranch commented Aug 14, 2018

@johnnyreilly can the comparison test’s "patch" feature be used to patch the test's webpack.config.js itself?

I'm thinking about the scenarios that need to be tested, and they include all three modes of creating a TS program—transpileOnly, experimentalWatchApi, and the normal language service route. Would you create a different comparison test for each even though it shouldn't theoretically change the output at all? Or is that something that's more appropriate for an execution test?

It's also definitely going to be worth testing the error cases, as an unbuilt project reference would compile perfectly fine if project references weren’t working, if that makes sense (TypeScript would just compile the source files per usual, instead of saying “hey this is part of a project reference that’s not built”). Are comparison tests useful for asserting failed builds at all?

Edit: I see the Webpack output is captured in a text file, so that looks pretty good for testing the error cases 👍

@andrewbranch
Copy link
Contributor

andrewbranch left a comment

Did a first pass at a comparison test. Not sure why there wasn't a .transpiled version generated like some of the others have though?

To generate outputs for older TS versions, do I need to manually install each older version and run again?

"composite": true
},
"files": [
"./index.ts"

This comment has been minimized.

@andrewbranch

andrewbranch Aug 14, 2018

Contributor

For some reason, without files or include, program.getProjectReferences()[0].commandLine.fileNames was empty. I thought it should have resolved to all *.ts files by default?

This comment has been minimized.

@johnnyreilly

johnnyreilly Aug 14, 2018

Member

To generate outputs for older TS versions, do I need to manually install each older version and run again?

No - we only run comparison tests for the latest TypeScript version. So 3 alone is fine

This comment has been minimized.

@johnnyreilly

johnnyreilly Aug 14, 2018

Member

For some reason, without files or include, program.getProjectReferences()[0].commandLine.fileNames was empty. I thought it should have resolved to all *.ts files by default?

Without project references in the mix then that should be the case. Is it different when rolling with project references perhaps?

This comment has been minimized.

@johnnyreilly

johnnyreilly Aug 14, 2018

Member

Regards the lack of transpile; that's surprising but I wouldn't worry right now. This is the code that generates it BTW: https://github.com/TypeStrong/ts-loader/blob/master/test/comparison-tests/create-and-execute-test.js

This comment has been minimized.

@andrewbranch

andrewbranch Aug 14, 2018

Contributor

When I was testing ad-hoc, the reference.commandLine.fileNames was working correctly, but I think I was using typescript@next... I'll have to investigate more.

This comment has been minimized.

@andrewbranch

andrewbranch Aug 14, 2018

Contributor

Ah, I was mistaken:

All implementation files must be matched by an include pattern or listed in the files array. If this constraint is violated, tsc will inform you which files weren’t specified

https://www.typescriptlang.org/docs/handbook/project-references.html#composite

It seems it's working as intended, but via our programmatic route we don't get that warning. Maybe need to add our own.

This comment has been minimized.

@johnnyreilly

johnnyreilly Aug 15, 2018

Member

can the comparison test’s "patch" feature be used to patch the test's webpack.config.js itself?

No I'm afraid not; the tests rely upon an unchanged webpack config between patches. Sorry

This comment has been minimized.

@andrewbranch

andrewbranch Aug 20, 2018

Contributor

It seems it's working as intended, but via our programmatic route we don't get that warning. Maybe need to add our own.

I'm not sure if this can be done reliably and efficiently... without actually doing a build of the dependent projects, it's hard to tell if a given file was intended to be included by one of them, or was just intended to be processed by the parent project (i.e. in our case, run through the loader). Implementing a warning from ts-loader side while consuming project references might be out of scope—hopefully it's something users will notice while actually running tsc on those projects, since pre-building the referenced projects is currently required, and tsc will emit the error if files or include don't cover all the files.

andrewbranch added some commits Aug 9, 2018

@andrewbranch andrewbranch force-pushed the andrewbranch:project-references branch from 992348c to d262970 Aug 20, 2018

@andrewbranch

This comment has been minimized.

Copy link
Contributor

andrewbranch commented Aug 20, 2018

@johnnyreilly status update:

Implementation

  • Waiting on Microsoft/TypeScript#26421 (no movement there since opening 😕)
  • Anything else? Feel free to do a closer review now; I think it's pretty much done!

Tests

  • Is it a problem that the .transpiled one didn't show up for the projectReferences and projectReferencesNoSourceMap comparison tests?
  • Should I copy any of the three comparison tests I added and run with experimentalWatchApi?
  • Any other comments/feedback on tests?
@johnnyreilly

This comment has been minimized.

Copy link
Member

johnnyreilly commented Aug 28, 2018

Sorry for the delay in responding @andrewbranch - I'm away and just have my phone and poor connectivity. In fact I'm typing this surrounded by 🐑 whilst lost rambling!

Is it a problem that the .transpiled one didn't show up for the projectReferences and projectReferencesNoSourceMap comparison tests?

I'm not sure it is; my guess is that people will only use projectReferences with non-transpile mode. Either way this is fine for now in my view.

Should I copy any of the three comparison tests I added and run with experimentalWatchApi?

Thanks but no. What I actually mean to do at some point is tweak the comparison test pack so it runs in 3 modes: standard, transpileOnly and watchApi. So in the longer term separate tests for the watchApi shouldn't exist. It's just another part of the test matrix if you will.

Any other comments/feedback on tests?

They're not passing 😂

Jokes aside, I wouldn't worry about the tests for now. First get to an implementation that you're content with, let's review that together and then we can work together on the test that cover this new functionality until they are fully passing / covering the scenarios we care about.

Thanks again for all the work you're doing; I really appreciate it. Much ♥️🌻

I should be back online in a week or so. Assuming I find the way out of this field 😉

@andrewbranch

This comment has been minimized.

Copy link
Contributor

andrewbranch commented Aug 28, 2018

Thanks! Hope you are enjoying your vacation.

The build is failing because I wrote a line that assumes Microsoft/TypeScript#26421 will be merged. I think maybe we just need to press on without that for now. I'll give it a few more days but have something mergeable by the time you're back.

program &&
program
.getProjectReferences()!
.find(ref => ref!.commandLine.fileNames.indexOf(filePath) > -1)

This comment has been minimized.

@ajafff

ajafff Aug 29, 2018

do you handle missing referenced projects somewhere else or how can you be sure ref is always defined?
ref will be undefined if a referenced project cannot be resolved, i.e. missing folder or missing tsconfig.json.

This comment has been minimized.

@andrewbranch

andrewbranch Sep 1, 2018

Contributor

It will be a holey array? I didn't realize that; it feels a bit unintuitive, but I guess the types agree. Let me do something here. Thanks for the feedback!

const { outputText, sourceMapText } = options.transpileOnly
? getTranspilationEmit(filePath, contents, instance, loader)
: getEmit(rawFilePath, filePath, instance, loader);
const jsFileName = instance.compiler.getOutputJavaScriptFileName(

This comment has been minimized.

@ajafff

ajafff Aug 29, 2018

given that the whole tsbuild API is now internal, this uses an internal function? I couldn't find a custom implementation if it in this PR

@ajafff

This comment has been minimized.

Copy link

ajafff commented Sep 20, 2018

@andrewbranch that change was published in typescript@next a few days ago

@johnnyreilly

This comment has been minimized.

Copy link
Member

johnnyreilly commented Sep 20, 2018

Thanks @ajafff! All yours @andrewbranch!

@andrewbranch

This comment has been minimized.

Copy link
Contributor

andrewbranch commented Sep 20, 2018

@andrewbranch that change was published in typescript@next a few days ago

This doesn't seem to be the case insofar as what npm i typescript@next does. There’s no instance of getResolvedProjectReference in node_modules/typescript/lib/typescript.js. I also tried installing typescript@master and building from source, but master fails to build for me.

At any rate, ddc5cc7 runs getResolvedProjectReferences instead of getProjectReferences if it exists, so I think that should be ok.

Am I free to hack on your branch?

By all means!

I'll work on ignoring the comparison test on Windows. I'll also write some execution tests so we have some in place.

🙏

@ajafff

This comment has been minimized.

Copy link

ajafff commented Sep 20, 2018

I'm already using program.getResolvedProjectReferences() and it compiles just fine with typescript@3.1.0-dev.20180915

https://github.com/fimbullinter/wotan/blob/0518a9a78ed0f38c490cde1533b00d7681821c99/packages/wotan/src/runner.ts#L327-L330

@andrewbranch

This comment has been minimized.

Copy link
Contributor

andrewbranch commented Sep 21, 2018

Ok, cool, thanks @ajafff. Confirmed that typescript@3.1.0-dev.20180915 works with the new API; typescript@3.1.0-dev.20180920 I think was just itself broken ¯\(ツ)

Working on the README now.

@andrewbranch

This comment has been minimized.

Copy link
Contributor

andrewbranch commented Sep 21, 2018

@johnnyreilly as I'm writing this README, I'm thinking that it honestly sounds kind of silly at this point without yet having support for building upstream projects. I think probably very few people will want to try out the feature before there's a solid solution for having everything built for you.

A somewhat interesting effect of ts-loader not yet supporting project references in master is that people who have project references in their tsconfig and are using ts-loader on those projects today are not actually using project references, but everything should be working perfectly for them, because each file request is just getting resolved to the source file, just as if they didn't have references in their tsconfig at all.

If we ship this now, enabled by default, without support for rebuilding upstream projects, we'll technically break those people, because all of a sudden, project references will start half-working instead of being ignored. Folks may not realize that ts-loader stopped building their files.

I'm thinking we need to put all this work behind a loader option. What do you think?

@johnnyreilly

This comment has been minimized.

Copy link
Member

johnnyreilly commented Sep 21, 2018

Oooh - good catch! Yes that makes sense. If you could include that information in the README that would be good. What do you think would be a good name for the feature flag loader option? projectReferences?

I'll probably write a blog post when this ships to publicise that this feature is available and that it's opt-in. I'll plan to use some of your README tweaks for part of that (giving credit where it's due of course ❤️ 😄 )

Working on ignoring the test on Windows now.

@andrewbranch

This comment has been minimized.

Copy link
Contributor

andrewbranch commented Sep 21, 2018

Awesome. After so much work, it's slightly disappointing that it’s not super usable yet, but I really think the hardest part is done. (🤞🙈) I may not be able to pick back up right away, but if nobody else jumps on it, I’ll want to see it through and figure out building upstream projects. That said, I’d be thrilled if someone else does want to champion that part, but my own sense of sunk cost won’t let me leave it half-done. 😄

@johnnyreilly

This comment has been minimized.

Copy link
Member

johnnyreilly commented Sep 21, 2018

Awesome. After so much work, it's slightly disappointing that it’s not super usable yet, but I really think the hardest part is done. (🤞🙈)

Completely agree.

I may not be able to pick back up right away, but if nobody else jumps on it, I’ll want to see it through and figure out building upstream projects. That said, I’d be thrilled if someone else does want to champion that part, but my own sense of sunk cost won’t let me leave it half-done. 😄

😄

@johnnyreilly

This comment has been minimized.

Copy link
Member

johnnyreilly commented Sep 21, 2018

Working on ignoring the test on Windows now.

I'd forgotten just how fiddly / flaky the comparison test pack can be.

Decided to use the opportunity to introduce a whole suite of fancy logic around ignoring tests. Managed to break the existing test pack in the process :facepalm:

Totally unnecessary.

Will replace https://github.com/TypeStrong/ts-loader/blob/master/test/comparison-tests/create-and-execute-test.js#L55 with something as simple as:

if (testName !== 'projectReferencesOutDir' || require('os').platform() !== 'win32') {
        // @ts-ignore
        it('should have the correct output', createTest(testToRun, testPath, {}));
}

Not tested this yet; wrote it on a Google Pixel on a train whilst reading the node docs on a spotty connection. 😄

Heading into work now; will try and take another look tonight / this weekend

@johnnyreilly

This comment has been minimized.

Copy link
Member

johnnyreilly commented Sep 21, 2018

Cool - that worked. I'll see if I can make a start on execution tests this weekend.

johnnyreilly added some commits Sep 22, 2018

@johnnyreilly

This comment has been minimized.

Copy link
Member

johnnyreilly commented Sep 23, 2018

Hey @andrewbranch ,

It's time to 🚢! I'm planning to push out this as version 5.2.0. I'll put a note in the README about the outDir Windows issue and probably put together an example repo too.

I'm going to write a blogpost to let people know about this new feature. I'm planning to plagiarise your excellent update to the README pretty heavily. I hope that's okay!

Thanks again for all your hard work. It's greatly appreciated! Much ❤️

@johnnyreilly johnnyreilly merged commit 635f745 into TypeStrong:master Sep 23, 2018

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@andrewbranch

This comment has been minimized.

Copy link
Contributor

andrewbranch commented Sep 23, 2018

Woop! 🙌🏼 Thanks for all the support throughout the process. This has been fun! Plagiarize away! 😄

@johnnyreilly

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment