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 the new watch api of compiler #685

Merged
merged 7 commits into from
Jan 20, 2018
Merged

Using the new watch api of compiler #685

merged 7 commits into from
Jan 20, 2018

Conversation

sheetalkamat
Copy link
Contributor

@sheetalkamat sheetalkamat commented Nov 28, 2017

This works with the changes of microsoft/TypeScript#20234

@johnnyreilly
Copy link
Member

Thanks so much for doing this - I really appreciate the effort! I'll take a look as soon as I get a moment.

@johnnyreilly
Copy link
Member

I was able to get this up and running locally by compiling the builderApi branch of TypeScript and npm linking it to ts-loader. I had to drop the strictFunctionTypes setting in the tsconfig.json - I'm guessing that you started work on this prior to that shipping?

On the face of it the changes look great!

It's hard to know how significant the performance gain as there's no tests specifically measuring performance. (Most of the time taken in the test runs is preparing the environment to run a test and cleaning up rather than anything else.) However, running some fairly rudimentary tests on my laptop seems to indicate it's faster using the new API.

The only question I really have is this: (copied across my question from the other thread)

  • ts-loader uses custom module resolution, which means we wont have information of failed lookup locations to watch, hence we wont know if we are updating the program correctly or not.

I'm guessing you're referring to the code here? If memory serves, ts-loader had custom module resolution from @jbrantly's original work. You'll see that we use the TypeScript resolution in certain circumstances. There's multiple strategies in play for backwards compatibility reasons.

With the next major version of ts-loader I was planning to drop support for TypeScript <= 2.3 to simplify the codebase. I was also planning to look again at whether we could just use the TypeScript module resolution and drop the custom resolution entirely. (Again it would be simpler and more consistent.)

Am I right in understanding that using the PR without that change could be problematic? Or is that going too far?

Thanks once again for your efforts - brilliant work!! 🌻

@HerringtonDarkholme
Copy link
Contributor

@jbrantly iirc custom module resolver used to be a bridge for enhanced resolver and typescript. Back then TS didn't have path mapping. So some mapping is done by tsloader.

@johnnyreilly johnnyreilly merged commit 7add160 into TypeStrong:master Jan 20, 2018
@johnnyreilly
Copy link
Member

Tests aren't passing as yet - there's multiple reasons for that. Merging to make it easier to work on.

@johnnyreilly
Copy link
Member

johnnyreilly commented Jan 20, 2018

Will look to release this with 3.3 - initially hidden behind the experimentalWatchApi option until some decent testing has taken place

@johnnyreilly
Copy link
Member

johnnyreilly commented Jan 21, 2018

Hey @sheetalkamat,

I've merged your PR and pushed out a new version of ts-loader adding the watch support behind a new option called experimentalWatchApi (which is disabled by default). It looks like there's some problems. I've raised this issue here to provide details:

microsoft/TypeScript#21325

I've also created a minimal illustration repo to demonstrate the problems:

https://github.com/johnnyreilly/typescript-ts-loader-watch-api-illustration

I'm highly motivated to get these issues resolved - if I can help in any way then please do tell me what I can do.

Thanks again for all your hard work - I really appreciate it.

@sheetalkamat
Copy link
Contributor Author

I will look into this either later today or tomorrow and get back to you. Thanks.

@johnnyreilly
Copy link
Member

👍 thanks so much!

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.

3 participants