Skip to content
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

Using Solution builder to build project references #935

Merged
merged 13 commits into from Sep 10, 2019
Merged

Using Solution builder to build project references #935

merged 13 commits into from Sep 10, 2019

Conversation

sheetalkamat
Copy link
Contributor

@sheetalkamat sheetalkamat commented May 16, 2019

This is based off microsoft/TypeScript#31432

@johnnyreilly
Copy link
Member

johnnyreilly commented May 16, 2019

Very exciting! Looks like there's a compilation error at the moment - I'm guessing we're just waiting for the relevant API to appear in @next 😁

@sheetalkamat
Copy link
Contributor Author

sheetalkamat commented May 16, 2019

@johnnyreilly Yes. The PR is almost ready for review and once the build should be able to succeed.

@johnnyreilly
Copy link
Member

johnnyreilly commented May 16, 2019

Cool - let me know when you're ready and we've a passing build and I'll take a look.

I'm guessing the new execution test you've added should pass with the typescript@next test run.

Copy link
Contributor

@andrewbranch andrewbranch left a comment

This is looking great! Next week I can try it out on a real life project before we merge. Super excited for this 🎉

src/config.ts Outdated Show resolved Hide resolved
src/servicesHost.ts Outdated Show resolved Hide resolved
@sheetalkamat sheetalkamat marked this pull request as ready for review Jun 6, 2019
@sheetalkamat
Copy link
Contributor Author

sheetalkamat commented Jun 6, 2019

@andrewbranch Can you please try it out now. SolutionBuilder api is in typescript nightly now and I have updated this PR.

@sheetalkamat
Copy link
Contributor Author

sheetalkamat commented Jun 6, 2019

@johnnyreilly This is ready to go in. Please take a look. Thanks.

@johnnyreilly
Copy link
Member

johnnyreilly commented Jun 6, 2019

Thanks @sheetalkamat! If I get time I'll try to look this weekend.

Am I right in thinking the SolutionBuilder api will be released with TypeScript 3.6?

Also I noticed your new test is being skipped right now:

Skipping test 3.6.0_projectReferencesToBeBuilt as its minimum version of 3.6.0 is greater than our current version of TypeScript: 3.6.0-dev.20190606

https://travis-ci.org/TypeStrong/ts-loader/jobs/542402642

@sheetalkamat
Copy link
Contributor Author

sheetalkamat commented Jun 6, 2019

@johnnyreilly Interesting not sure what I can do to fix that, but I can run that as a single test and it passes.

@andrewbranch
Copy link
Contributor

andrewbranch commented Jun 7, 2019

I was blocked by microsoft/TypeScript#31792 trying to test this in the wild yesterday, but should be able to play with it today.

Copy link
Contributor

@andrewbranch andrewbranch left a comment

I’m now pseudo-blocked by #949 (I can observe your changes working, but I don’t get a clean build), but the first thing I noticed was that we don’t create the solution builder if transpileOnly is true. This is how the majority of ts-loaders configure the loader, so we need to do something for them.

When we talked about this before, I think you said that we can’t skip type checking the referenced projects (as transpileOnly would suggest) before we emit them, but I think that’s ok for now.

When #949 is fixed (by #945), I should be able to get a clean build and test the end-to-end experience. I might also have some time Monday to add this to transpileOnly if you’re busy. Thanks!

@johnnyreilly
Copy link
Member

johnnyreilly commented Jun 8, 2019

It probably makes more sense for @andrewbranch to carry on with the reviewing / merging of this. He's much more knowledgeable about project references than I am.

One question (and it sounds like @andrewbranch is thinking of this too): how does projectReferences support look for users who are using transpileOnly: true and handing off type checking to fork-ts-checker-webpack-plugin?

Does that just work as things stand, or not? I haven't given it a try myself... BTW I'm an admin on that project too and it is also completely open to contributions! ❤️

If he's happy then I'm happy 😀

src/servicesHost.ts Outdated Show resolved Hide resolved
@artem-malko
Copy link

artem-malko commented Jul 7, 2019

Hi all)
Any news?

src/config.ts Outdated Show resolved Hide resolved
src/config.ts Outdated Show resolved Hide resolved
@andrewbranch
Copy link
Contributor

andrewbranch commented Sep 10, 2019

Hey all! I owe you an apology. I was confused on the state of this PR 😬

I had intended to try this PR out end-to-end while we were still waiting on 3.6 to land and while Sheetal was wrapping things up. Other issues (and my workload for TypeScript 3.7) are still making that difficult, but I totally missed the fact that Sheetal was done here. I was confused by the fact that the build is still red (which as @AndrewKirkovski pointed out, looks to be just an infrastructure hiccup) and by the fact that package.json is still referencing the nightly. I’ve just pushed an update fixing that. We probably could have merged this when 3.6 stable was released, and it was totally my fault for not paying close enough attention. I’m really sorry to everyone who was anxiously awaiting this feature.

I’ll also be updating the changelog shortly, then I think we can merge this.

In the meantime, has anyone else tried this PR in a real project? That’s certainly the single most helpful thing anyone can do with any large PR like this.

@kirill-konshin
Copy link

kirill-konshin commented Sep 10, 2019

Does this PR address incremental build too?

@andrewbranch
Copy link
Contributor

andrewbranch commented Sep 10, 2019

@kirill-konshin yes (notice that .tsbuildinfo is added to the .gitignore of the execution tests—that’s because the build info file is getting generated 🎉)

@kirill-konshin
Copy link

kirill-konshin commented Sep 10, 2019

@andrewbranch yes, I just received your comment in incremental-related issue! Perfecto!

@johnnyreilly
Copy link
Member

johnnyreilly commented Sep 10, 2019

I’ll also be updating the changelog shortly, then I think we can merge this.

Nice work @andrewbranch! Thanks so much!

If you're happy with this do feel free to merge and push out a release (just add an entry to the release to the https://github.com/TypeStrong/ts-loader/releases and the GitHub action should take care of the rest)

@paulvanbrenk
Copy link

paulvanbrenk commented Sep 10, 2019

Thanks @andrewbranch & @sheetalkamat

@andrewbranch andrewbranch merged commit bfa48c7 into TypeStrong:master Sep 10, 2019
0 of 2 checks passed
@andrewbranch
Copy link
Contributor

andrewbranch commented Sep 10, 2019

This is in the process of shipping as v6.1.0. My current thinking is to get some validation that things are generally working, then remove the projectReferences configuration option as a v7.0 if things look good / after any bug fixes from early feedback. I think there should be no reason to toggle project references on or off based on loader config when tsconfig is a perfectly good source of truth, but removing the option will be a breaking change, and I want to be extra sure the functionality is solid before removing the opt-in nature of the feature.

@ericanderson
Copy link

ericanderson commented Sep 10, 2019

Due to random bugs that can happen, leaving the failsafe so regular tsc can use build references but webpack doesn’t have to is good. Maybe 7 could default to on?

@andrewbranch
Copy link
Contributor

andrewbranch commented Sep 10, 2019

I think you’d be better off using a different tsconfig for ts-loader that makes the intention more explicit. In general, I want to move ts-loader away from specialized behavior that contradicts the tsconfig it’s reading. Removing the option from ts-loader doesn’t remove the option for the user, it just shifts it onto TypeScript itself.

@sublimator
Copy link

sublimator commented Sep 11, 2019

I have a (unfortunately not open source, yet ...) project that is using projectReferences: true, and for some reason webpack (using webpack-cli) stalls and doesn't exit after what seems like a successful build.

Not sure how to provide more info:

    "ts-loader": "^6.1.0",
    "webpack": "^4.39",
    "webpack-cli": "^3.3.8",
    "ts-loader": "^6.1.0",

I happen to be using TypeScript for the webpack config itself, which may or may not indicate something.

@sublimator
Copy link

sublimator commented Sep 11, 2019

I just cloned this repository and "manually" ran webpack on the test/execution-tests/3.6.0_projectReferencesToBeBuilt and experienced the same hang. Note that I'm on MacOS Mojave yet the same hang is evident on CircleCI running linux inside docker (circleci/node:10.16.2-buster-browsers to be exact)

@sublimator
Copy link

sublimator commented Sep 11, 2019

Running yarn test from the root of the cloned repo

  projectReferences
    1) should have the correct output
    ✓ should work with transpileOnly (822ms)


  1 passing (4s)
  1 failing

  1) projectReferences
       should have the correct output:

      Uncaught AssertionError [ERR_ASSERTION]: bundle.js is different between actual and expected
      + expected - actual

       exports.lib = {
           one: 1,
           two: 2,
           three: 3
      +    / I am adding this comment here by hand to ensure
      +    / Webpack is using the JS output for project references
       };
       /# sourceURL=webpack://./lib/index.ts?");
       /***/ })
       /******/ });
      
      at compareActualAndExpected (test/comparison-tests/create-and-execute-test.js:467:16)
      at /Users/nicholasdudfield/projects/ts-loader/test/comparison-tests/create-and-execute-test.js:333:13
      at Array.forEach (<anonymous>)
      at compareFiles (test/comparison-tests/create-and-execute-test.js:329:31)
      at Watching.handler (test/comparison-tests/create-and-execute-test.js:194:9)
      at compiler.hooks.done.callAsync (node_modules/webpack/lib/Watching.js:100:9)
      at AsyncSeriesHook.eval [as callAsync] (eval at create (node_modules/webpack/node_modules/tapable/lib/HookCodeFactory.js:32:10), <anonymous>:6:1)
      at AsyncSeriesHook.lazyCompileHook (node_modules/webpack/node_modules/tapable/lib/Hook.js:154:20)
      at Watching._done (node_modules/webpack/lib/Watching.js:99:28)
      at compiler.emitRecords.err (node_modules/webpack/lib/Watching.js:73:19)
      at Compiler.emitRecords (node_modules/webpack/lib/Compiler.js:366:39)
      at compiler.emitAssets.err (node_modules/webpack/lib/Watching.js:54:20)
      at hooks.afterEmit.callAsync.err (node_modules/webpack/lib/Compiler.js:352:14)
      at AsyncSeriesHook.eval [as callAsync] (eval at create (node_modules/webpack/node_modules/tapable/lib/HookCodeFactory.js:32:10), <anonymous>:6:1)
      at AsyncSeriesHook.lazyCompileHook (node_modules/webpack/node_modules/tapable/lib/Hook.js:154:20)
      at asyncLib.forEach.err (node_modules/webpack/lib/Compiler.js:349:27)
      at done (node_modules/neo-async/async.js:2854:11)
      at /Users/nicholasdudfield/projects/ts-loader/node_modules/neo-async/async.js:2805:7
      at /Users/nicholasdudfield/projects/ts-loader/node_modules/graceful-fs/graceful-fs.js:45:10
      at FSReqWrap.oncomplete (fs.js:141:20)

Likewise yarn test does not seem to exit and stalled after the above output

@johnnyreilly
Copy link
Member

johnnyreilly commented Sep 11, 2019

@sublimator to do anything useful to help we're going to need a minimum reproduction repo please. If you can make one, raise an issue and link back to this with it that would be amazing.

@sublimator
Copy link

sublimator commented Sep 11, 2019

@johnnyreilly

Hey, will do!

But as stated above, all you need to repro is clone this repo and run the tests, either with yarn test, or manually invoking webpack on the "fixture", and it hangs

@AndrewKirkovski
Copy link

AndrewKirkovski commented Sep 12, 2019

I have identical hanging to @sublimator happening on Windows 10 machine

@johnnyreilly
Copy link
Member

johnnyreilly commented Sep 12, 2019

Please track this here @AndrewKirkovski:

#998

@maclockard
Copy link

maclockard commented Sep 17, 2019

Definitely still curious what the expected interaction between ts-loader with projectReferences: true and fork-ts-checker-webpack-plugin is. Will fork-ts need to add project reference support as well?

@johnnyreilly
Copy link
Member

johnnyreilly commented Sep 17, 2019

I suspect so, yes. If you'd like to raise a PR we'd appreciate it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet