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

Fix SolutionBuilder watches #1003

Merged
merged 19 commits into from Sep 19, 2019
Merged

Fix SolutionBuilder watches #1003

merged 19 commits into from Sep 19, 2019

Conversation

sheetalkamat
Copy link
Contributor

@sheetalkamat sheetalkamat commented Sep 13, 2019

Fixes #998

src/instances.ts Outdated
{ verbose: true, watch: true }
{ verbose: true }
);
loader._compiler.hooks.watchRun.tap(
Copy link
Contributor Author

@sheetalkamat sheetalkamat Sep 13, 2019

Choose a reason for hiding this comment

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

I am not sure if this is correct way to do cleanup before watch compilation begins.. Also I think we need to create solutionbuilder host and solutionBuilder both again but I don't know how to check that..

Copy link
Contributor Author

@sheetalkamat sheetalkamat Sep 13, 2019

Choose a reason for hiding this comment

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

Edit i checked this isnt right way to do this. I think we need to create with watch host when running with --watch. Can someone let me know how to detect that here?

Copy link
Contributor

@andrewbranch andrewbranch Sep 13, 2019

Choose a reason for hiding this comment

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

There should be a watchMode property on the webpack Compiler object: https://github.com/webpack/webpack/pull/8253/files

Copy link
Contributor Author

@sheetalkamat sheetalkamat Sep 13, 2019

Choose a reason for hiding this comment

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

I think even with that we need to implement watchFile as loader.addDepencedency but that needs more work I think given there are too many places clearing dependencies...

@johnnyreilly
Copy link
Member

johnnyreilly commented Sep 13, 2019

Thanks for this @sheetalkamat!

Copied across from linked issue:

I remembered that this was todo.. While in normal build scenario non watch makes sense but we would want to handle --watch portion of it since webpack wont know about the referenced project's files (to watch).

From looking at the changes here it mostly seems that be dropping usage of the watch APIs. Swapping from createSolutionBuilderWithWatchHost to createSolutionBuilder.

Whilst this fixes the webpack not exiting with project references issue does this mean that watch mode doesn't work for project references with this PR? i.e. If I make a change to a dependent project with this change in place, does that mean that changes will not be picked up / builds retriggered?

I'm not totally clear on the intent of this PR and so wanted to clarify.

Thanks again so much for your time and work!

@Bessonov
Copy link

Bessonov commented Sep 13, 2019

Run in the same issue with hanging webpack. Tested this PR (a2a37c). Just building works - webpack exists. But if webpack is in watch mode, then recompilation works only for the current project, but not for referenced one. Does it make sense to synchronize webpack watch mode with typescript watch mode?

@sublimator
Copy link

sublimator commented Sep 13, 2019

Does it make sense to synchronize webpack watch mode with typescript watch mode?

On the surface it would seem so, no?

Are there other contexts other than webpack need to consider as well ? (Personally only ever used ts-loader with webpack)

@sheetalkamat
Copy link
Contributor Author

sheetalkamat commented Sep 13, 2019

I am looking into supporting watch mode separately. This mean while should unblock people that just run webpack

@sublimator
Copy link

sublimator commented Sep 13, 2019

@sheetalkamat

Makes sense, sounds good to me :)

src/instances.ts Outdated
);
// TODO:: handle --watch?

Choose a reason for hiding this comment

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

Ja, if it's tricky, maybe just should roll this out in the short term

@Bessonov
Copy link

Bessonov commented Sep 13, 2019

I can unblock with kill, but I can't develop without watch mode. Therefore the watch mode is more critical for me than unblocking. But I'm fine if the next version unblocks and the next one fix the watch mode 👍

@sublimator
Copy link

sublimator commented Sep 13, 2019

@Bessonov
Hanging is a bit painful on CI, but yeah --watch would be --pretty-please-terrific!

@johnnyreilly
Copy link
Member

johnnyreilly commented Sep 13, 2019

I'm happy for this to be merged and released as v6.1.1 - is there anything more to do?

If not, would you like to update the package.json / CHANGELOG.md?

Done it; love that GitHub makes editing a PRs branch possible by default now!

Are we good to merge this when Travis / AppVeyor are finished?

@sublimator
Copy link

sublimator commented Sep 13, 2019

image

Ran the branch and it fails on the first invokation.
Works on the subsequent?

@sublimator
Copy link

sublimator commented Sep 13, 2019

➜  3.6.0_projectReferencesToBeBuilt git:(fixWatch) ✗ rm lib/tsconfig.tsbuildinfo 
➜  3.6.0_projectReferencesToBeBuilt git:(fixWatch) ✗ rm lib/index.js.map 
➜  3.6.0_projectReferencesToBeBuilt git:(fixWatch) ✗ rm lib/index.d.ts 
➜  3.6.0_projectReferencesToBeBuilt git:(fixWatch) ✗ node ../../../node_modules/.bin/webpack-cli --config webpack.config.js
Hash: 1723e4ce27cdd156732c
Version: webpack 4.20.2
Time: 2933ms
Built at: 09/14/2019 12:36:30 AM
    Asset      Size  Chunks             Chunk Names
bundle.js  4.27 KiB    main  [emitted]  main
Entrypoint main = bundle.js
[./src/app.ts] 518 bytes {main} [built] [failed] [1 error]

ERROR in ./src/app.ts
Module build failed (from /Users/nicholasdudfield/projects/ts-loader/index.js):
Error: TypeScript emitted no output for /Users/nicholasdudfield/projects/ts-loader/test/execution-tests/3.6.0_projectReferencesToBeBuilt/src/app.ts.
    at makeSourceMapAndFinish (/Users/nicholasdudfield/projects/ts-loader/dist/index.js:78:15)
    at successLoader (/Users/nicholasdudfield/projects/ts-loader/dist/index.js:68:9)
    at Object.loader (/Users/nicholasdudfield/projects/ts-loader/dist/index.js:22:12)

(edit: broke up this for easier reading)

➜  3.6.0_projectReferencesToBeBuilt git:(fixWatch) ✗ node ../../../node_modules/.bin/webpack-cli --config webpack.config.js
Hash: 44ad6121c445f269b58c
Version: webpack 4.20.2
Time: 2207ms
Built at: 09/14/2019 12:36:35 AM
    Asset      Size  Chunks             Chunk Names
bundle.js  4.39 KiB    main  [emitted]  main
Entrypoint main = bundle.js
[./lib/index.ts] 97 bytes {main} [built]
[./src/app.ts] 221 bytes {main} [built]
➜  3.6.0_projectReferencesToBeBuilt git:(fixWatch) ✗ 

Hrmm, something is up

@sublimator
Copy link

sublimator commented Sep 13, 2019

image

And if you delete just one of the js files it gets "stuck", unable to recover with the error above

@johnnyreilly
Copy link
Member

johnnyreilly commented Sep 13, 2019

Thanks for testing and reporting with detail @sublimator. @sheetalkamat - it looks like there's still issues.

@sheetalkamat
Copy link
Contributor Author

sheetalkamat commented Sep 13, 2019

Looking into it

@sheetalkamat
Copy link
Contributor Author

sheetalkamat commented Sep 13, 2019

Note:: This should fix initial thing but still working on it.. (reporting error and I think watch)

@sheetalkamat
Copy link
Contributor Author

sheetalkamat commented Sep 13, 2019

Are there tests reporting error that I can see sample of to add test to it

@sheetalkamat
Copy link
Contributor Author

sheetalkamat commented Sep 13, 2019

Also note that I know there are tests for project references failing on older build because I was trying to work clean slate.. Once this work is stabilized, will add the deleted code back.

@sublimator
Copy link

sublimator commented Sep 14, 2019

@sheetalkamat
Let me know when ready for some testing :)
Thanks!

@johnnyreilly
Copy link
Member

johnnyreilly commented Sep 14, 2019

Are there tests reporting error that I can see sample of to add test to it

I think what you're looking for is a comparison test. There's a specific test that you could clone if you wanted to create a new comparison test which specifically tests for an expected error case:

https://github.com/TypeStrong/ts-loader/tree/master/test/comparison-tests/errors

Details of how you can work with comparison tests can be found here: https://github.com/TypeStrong/ts-loader/blob/master/test/comparison-tests/README.md

Does that give you what you need?

@sheetalkamat
Copy link
Contributor Author

sheetalkamat commented Sep 16, 2019

@johnnyreilly Thank you for the pointer.. That's exactly what I needed..

@johnnyreilly
Copy link
Member

johnnyreilly commented Sep 17, 2019

We are green! Are you ready for this to be merged and released @sheetalkamat?

@sheetalkamat
Copy link
Contributor Author

sheetalkamat commented Sep 17, 2019

Will look into --watch today and if that takes longer, we can merge this? Will let you know..

@sheetalkamat
Copy link
Contributor Author

sheetalkamat commented Sep 18, 2019

Of course tsc -b recovers from this situation, so it would be an expectation that webpack/ts-loader can as well ?

tsc -b does not handle that scenario (Refer microsoft/TypeScript#30602)

c:\temp\test>node c:\TypeScript\built\local\tsc.js -b  --clean

c:\temp\test>node c:\TypeScript\built\local\tsc.js -b

c:\temp\test>dir main.js
 Volume in drive C is OSDisk
 Volume Serial Number is F41A-DBC9

 Directory of c:\temp\test

09/18/2019  08:19 AM                43 main.js
               1 File(s)             43 bytes
               0 Dir(s)  809,078,542,336 bytes free

c:\temp\test>del main.js

c:\temp\test>node c:\TypeScript\built\local\tsc.js -b

c:\temp\test>dir main.js
 Volume in drive C is OSDisk
 Volume Serial Number is F41A-DBC9

 Directory of c:\temp\test

File Not Found

c:\temp\test>

@sublimator
Copy link

sublimator commented Sep 18, 2019

@sublimator
Copy link

sublimator commented Sep 18, 2019

@sublimator
Copy link

sublimator commented Sep 18, 2019

@johnnyreilly
Copy link
Member

johnnyreilly commented Sep 18, 2019

@sublimator have you had chance to test with @sheetalkamat's latest changes? It sounds like we may be there, I'd love to get you to try it out and confirm.

@sheetalkamat
Copy link
Contributor Author

sheetalkamat commented Sep 18, 2019

@johnnyreilly I think this should be it for this PR,., This still doesn't emit .tsbuildinfo for current program when using experimentalWatchAPI( note its not possible to get the .tsbuildinfo from language service as its not meant for that). But that's a separate issue than supporting project references.
Also I had to copy few things from typescript but I will shortly open PR that will have the apis for those things..

@sheetalkamat
Copy link
Contributor Author

sheetalkamat commented Sep 18, 2019

@sublimator as mentioned at #1003 (comment) deleting js file from referenced project's output is not supported scenario even with tsc --b

@sublimator
Copy link

sublimator commented Sep 19, 2019

@sheetalkamat
Sorry, just got to my computer

I am referring to a leaf project as per:
image

Although, editing that test project to make the entry point "main" tsconfig.json have compilerOptions composite: true seems to work too??
(edit: actually, you don't even need to change the entry point to composite: true??)
image

Is this a windows vs MacOS thing ??

@sheetalkamat
Copy link
Contributor Author

sheetalkamat commented Sep 19, 2019

@sheetalkamat
https://github.com/microsoft/TypeScript/blob/master/src/compiler/diagnosticMessages.json#L4055-L4059

That is the message that indicates to build but does not really build.. Check out the microsoft/TypeScript#30602 as per my earlier comment. that's not something that would be fixed here.

@sublimator
Copy link

sublimator commented Sep 19, 2019

@sheetalkamat
image
I don't know why I'm getting different results than you, but it's definitely building for me ??

@johnnyreilly
Copy link
Member

johnnyreilly commented Sep 19, 2019

Let's get this merged and released. There's been a ton of progress here - thanks so much @sheetalkamat! If there's further things to do they can be addressed subsequently.

@johnnyreilly johnnyreilly merged commit ae0698d into TypeStrong:master Sep 19, 2019
2 checks passed
@sublimator
Copy link

sublimator commented Sep 19, 2019

Yeah, don't mean to come off as ungrateful :)
Just reporting my findings. I can't explain it ?? MacOS vs Windows differences ??
Having a play now with the 'sweet path'

@sublimator
Copy link

sublimator commented Sep 19, 2019

--watch mode seems to be working, and picking up changes!
normal mode seems to be working and picking up changes!

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.

5 participants