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

This is based off microsoft/TypeScript#31432

@johnnyreilly
Copy link
Member

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

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

@johnnyreilly
Copy link
Member

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 June 6, 2019 15:38
@sheetalkamat
Copy link
Contributor Author

@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

@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

@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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Hi all)
Any news?

src/config.ts Outdated Show resolved Hide resolved
src/config.ts Outdated Show resolved Hide resolved
@kirill-konshin
Copy link

Does this PR address incremental build too?

@andrewbranch
Copy link
Contributor

@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

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

@johnnyreilly
Copy link
Member

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

Thanks @andrewbranch & @sheetalkamat

@andrewbranch andrewbranch merged commit bfa48c7 into TypeStrong:master Sep 10, 2019
@andrewbranch
Copy link
Contributor

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

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

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

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

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

@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

@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

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

@johnnyreilly
Copy link
Member

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

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