-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
Api for creating program in watch mode and using builder to get incremental emit/semantic diagnostics #20234
Conversation
…changeset only after getting the result
…and skip the diagnostics check
This is amazing! cc @johnnyreilly |
This is awesome! @sheetalkamat would you be open to submitting a similar PR against ts-loader? I'd love to get this support into ts-loader as fast as possible (as I really, really like fast builds 😄 ) It'd be most welcome and I'd be happy to provide any assistance you might need along the way. (I can help slay test pack dragons 🐉 etc) Great work BTW! I'm really pleased the TypeScript team are doing this - thank you so much! 🌻 BTW your link to jrieken/gulp-tsb is broken... Think it should be jrieken/gulp-tsb#74 |
@piotr-oles this could be useful for fork-ts-checker-webpack-plugin perhaps? (In fact should make it much simpler I would guess?) |
@johnnyreilly I did create a branch for ts-loader but wasn't sure if that is improvement over current implementation or not.
I have still submitted PR with the work I had done at TypeStrong/ts-loader#685 |
Thanks so much - I'll have a tinker! |
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? |
@johnnyreilly yes thats the code.. while looking at it in deep there are cases when module resolution through typescript is used and sometimes not. Wasnt very clear on whats going on there.. Was not even sure if you would have information about locations to watch on for resolving module names to consider having that as part of the api. |
@sheetalkamat cool - this just reinforces my plan to drop custom module resolution and move to use TypeScript's entirely. My current plan is to wait until your TypeScript PR has been merged and so this functionality starts appearing in the nightly builds. Once that's in place I can then look to merge your ts-loader PR and potentially drop the custom module resolution at the same time. The code should become much clearer 👍 Have you any idea when this is likely to get merged? Thanks again for all your work x 💯 |
…program as part of this api
…o align with compiler host
…ost that combines the functionality
src/compiler/watch.ts
Outdated
/** | ||
* Creates the watch compiler host from system for config file in watch mode | ||
*/ | ||
export function createWatchCompilerHostOfConfigFile(configFileName: string, optionsToExtend: CompilerOptions | undefined, system: System, reportDiagnostic: DiagnosticReporter | undefined, reportWatchStatus: WatchStatusReporter | undefined): WatchCompilerHostOfConfigFile { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider combining these two APIs into createWatchCompilerHost
(jsut like we havecreateCompilerHost
) with two overloads instead of two separate functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
type WatchStatusReporter = (diagnostic: Diagnostic, newLine: string) => void; | ||
interface WatchCompilerHost { | ||
/** If provided, callback to invoke before each program creation */ | ||
beforeProgramCreate?(compilerOptions: CompilerOptions): void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: Can we replace beforeProgramCreate
and afterProgramCreate
with just createProgram
and make it required. this returns a Program
instance that can either be a regular Program
or a BuilderProgram
based on what the caller chooses?
this way we do not need to distinguish between createWatchBuilderProgram
and createWatchProgram
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But host is passed to createWatchProgram or createWatchBuilderProgram to actually create the program or BuilderProgram.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am thinking of something like
export function createWatchProgram<T extends Program = EmitAndSemanticDiagnosticsBuilderProgram>(host: WatchCompilerHostOfFilesAndCompilerOptions, createProgram : (old:T|undefined) =>T = createEmitAndSemanticDiagnosticsBuilderProgram): WatchOfFilesAndCompilerOptions<T>;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure i understood your answer. maybe easier to do this in person tommrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
builderApi...builderApiWithCreateProgram the change to take in createProgram as input on host.. I think it makes it little bit more messy.
/** | ||
* Creates the watch from the host for root files and compiler options | ||
*/ | ||
function createWatchBuilderProgram<T extends BuilderProgram>(host: WatchCompilerHostOfFilesAndCompilerOptions & BuilderProgramHost, createBuilderProgram: (newProgram: Program, host: BuilderProgramHost, oldProgram?: T) => T): WatchOfFilesAndCompilerOptions<T>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems that this is the default behavior of createWatchProgram
anyways, and to override that you need to change the onAfterProgramCreate
on the host.. if this is accurate, why do we need this helper?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in createWatchCompilerHost
we return an object whose afterProgramCreate
property is set to emitFilesAndReportErrorUsingBuilder
by default. this will create a builderProgram using createEmitAndSemanticDiagnosticsBuilderProgram
. what am i missing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But ts-loader or gulp-tsb do not use createWatchCompilerHost to create watch compiler host. Instead they create their own WatchCompilerHost that doesnt have these properties set.
752e9d7
to
1c1226e
Compare
1c1226e
to
2be231d
Compare
@mhegazy Updated as per our offline discussion |
@mhegazy Is this going to land in the 2.7 release? If so, niiiiice. |
@sheetalkamat - well done for all your work! So glad this has now landed! 🎆 Question: is any of the refactoring done as part of getting this merged into master likely to have an impact on your PR for ts-loader? |
@johnnyreilly the ts-loader PR is uptodate with the changes of this PR |
Thanks @sheetalkamat - I'll give it a whirl and report back! |
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 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. |
Example usage of the watch API as well as builder api is in jrieken/gulp-tsb#74
Example usage of the watch API and builder api to get semantic diagnostics only is in s-panferov/awesome-typescript-loader#519
The below api is to allow caller to create a program in watch mode that will watch the changes in files/options etc and update it. As a result, the emit happens only on the affected files incrementally and semantic diagnostics are cached as well resulting in getting new errors only for the affected files. (note the program reports diagnostics for whole program but recalculates semantic diagnostics for affected files)
The
system
andreportDiagnostics
if not provided defaults to usingts.sys
and writing diagnostics to usingsys.write
. This is optional parameter to allow callers of the api to provide own implementation of how to read/write filesThe returned result of these watch mode program is interface that has method to
getProgram
by synchronizing with changes in the host to emit files and/or report diagnostics if needed. Note that in the above api usage ofSystem
allows program to watch for changes and it would update itself on detecting changes in files/settings and so on. Note that program is synchronized on such updates throughSystem.setTimeout
so that it can batch the updates. The below method is way for user to ask for update on the program.Below api allows user to have more control on what to do after creation/update of the program as well as provide
CompilerHost
like api:The host implantation will allow the caller to provide callbacks to be called after and before program create through
beforeProgramCreate
andafterProgramCreate
. Example usage of these is theWatchHost
created bytsc
where onbeforeProgramCreate
enables statistics for the program and afterProgramCreate it reports those as well as diagnostics and emits files. The callbacks are optional and are useful if host allows synchronizing async after detecting changes (throughsetTimeout
andclearTimeout
). The host has most of the methods like CompilerHost, except it doesnt havegetSourceFile
api as sourceFile management is done inside the api itself. In addition toCompilerHost
like api, it haswatchFile
andwatchDirectory
methods that have to be provided, these are important so that WatchProgram is intelligent and efficient with reuse.In addition to this there are builder program api's that let you create program that can only emit affected files and cache the semantic diagnostics for files that havent changed or have same diagnostics.
There are two apis to create two types of the Builder, one that manages only semantic diagnostics and other one that manages semantic diagnostics as well as allows emitting only affected files.
The host takes in
createHash
as optional parameter wherein, it would be invoked to store the hash for the module shape of the files. If not provided, the text itself is stored (which can mean storing larger strings) and hencecreateHash
if possible is recommended. ThewriteFile
if provided, is used as fallback during emit if there is no local writeFile callback. Failing to provide this, the emit would invoke CompilerHost's writeFile callback.You can create new program by passing old Program just like
createProgram
api to get incremental program. Both builders also have methodgetAllDependencies
allows one to get all the dependencies for the file. Eg. usage it can be used to report those dependencies towebpack
.The other api's in BuilderProgram are just wire through program so that you dont need to store program and can just store BuilderProgram instead. Note that
getSemanticDiagnostics
andemit
are special depending on the type of the builder and will be discussed in detail further.The builder created by
createSemanticDiagnosticsBuilderProgram
has methodgetSemanticDiagnosticsOfNextAffectedFile
to iterate through changed files and get its semantic diagnostics and the affected source file the diagnostics are for. When any changed file results in affecting whole program the diagnostics are returned with the affected as program. When this method is called and there are no more changed/affected files that need to re-evaluate semantic diagnostics, it returnsundefined
notifying that the iteration is complete. The another methodgetSemanticDiagnostics
is available to get diagnostics for the source file but it is required that iteration through the program change is complete Only in case ofSemanticDiagnosticsBuilderProgram
if sourceFile is not provided, it will iterate through all affected files and cache the diagnostics. The purpose of this method is mainly to get diagnostics in the non-affected program so that the user of the API doesn't need to hold diagnostics from previous program and it can report all the errors instead of just affected file errors, or in case ofEmitAndSemanticDiagnosticsBuilderProgram
it can also be used to get all errors after emit.The builder created using
createEmitAndSemanticDiagnosticsBuilderProgram
allows you to iterate through changed files to emit the changed/affected files throughemitNextAffectedFile
. Just like earlier method to iterate through semantic diagnostics of the changed files, this method returns the emit result and affected as program or source file depending on the change resulting in the emit. Once iteration is complete the call toemitNextAffectedFile
returnundefined
.Apart from this there is also api that uses
Watch
andBuilderProgram
together to create easierWatch<BuilderProgram>
throughcreateWatchBuilderProgram
. With this api if you providecreateSemanticDiagnosticsBuilderProgram
orcreateEmitAndSemanticDiagnosticsBuilderProgram
as parametercreateBuilderProgram
everytime you querygetProgram
on thewatch
you would getBuilderProgram
of that kind instead of program.