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

Emitting .tsbuildinfo when using watch api #1012

Closed
wants to merge 8 commits into from
Closed

Emitting .tsbuildinfo when using watch api #1012

wants to merge 8 commits into from

Conversation

sheetalkamat
Copy link
Contributor

No description provided.

@johnnyreilly
Copy link
Member

Looks amazing @sheetalkamat! I'm certain this is going to make a bunch of people very happy indeed!

I think with this in place it might be worth considering doing a breaking changes release of ts-loader that makes using the watch API the default behaviour (people will still be able to turn off)

That should flush out if there's any outstanding issues with using the watch API in ts-loader whilst affording people a way to not use it if there are issues.

In terms of priorities I want to try and get Travis and AppVeyor running comparison tests again first, but this is very very much the next thing on the list!

#1011

@johnnyreilly
Copy link
Member

Hey @sheetalkamat ,

I haven't forgotten about this; just so you know! I've got a super busy week coming up and I'll try and loop back round to look at this the week after.

Just to give you a heads up, my thinking right now around this functionality is along these lines:

  1. As part of this, get tests running using both watch API and not
  2. Assuming all goes well, look to get this released as is
  3. Subsequently do a breaking changes release (v7.0.0 probably) which renames the flag from experimentalWatchApi to watchApi and makes that the default experience for ts-loader. This will mean people use this unless they opt out. I'm rather expecting that this could end up surfacing some issues - that's fine. If the issues are truly problematic then people have a way back.

@sheetalkamat sheetalkamat deleted the incrementalUsingWatchApi branch September 23, 2019 17:36
@RyanCavanaugh
Copy link

@johnnyreilly let's talk division of labor - we've sort of reached our limit of understanding on the ts-loader codebase for the moment and don't have resources to get to up to speed for getting the tests running in both modes. Would you be able to pick up on Sheetal's PR to get that implemented?

@johnnyreilly
Copy link
Member

johnnyreilly commented Sep 25, 2019

Sorry, that probably wasn't clear from my original message.

Yeah it's always been my plan that I will get tests running using both watch API and not. I'll also handle the breaking changes release of 7.0. My apologies for not making that more explicit; I wanted to convey an overall plan for things we should do with the project; this is not a worklist for @sheetalkamat or anyone else 😁

I'm hoping that I can hack on the PR that @sheetalkamat has open to get tests running in both modes (1.), that we can collaborate on that to get that to a place where we're happy to merge. I'll plan to handle 2. and 3. myself.

Sound good?

@RyanCavanaugh
Copy link

Perfect! Thanks for your help here. We're really excited about this.

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

3 participants