Open artem1458 opened 11 months ago
@nonara Hey, sorry for bothering, but do you have any thoughts on this one?
I confirm. I'll add that there's a similar issue with ForkTSChecker
I conducted some research. For example, I tried using the 'transformProgram': true option. The plugin code looked something like this:
export default function (program: Program, host: CompilerHost) {
const { ...newProgram } = program;
return Object.assign(program, {
getSemanticDiagnostics: () => {
// some code
return [];
}
});
}
And this works with tscp
, but still does not work with fork-ts-checker-webpack-plugin. I dug into the source code of fork-ts-checker-webpack-plugin and noticed that it receives an unpatched ts.Program. In this case, the creation happens using the createSemanticDiagnosticsBuilderProgram method. Perhaps, this method is not being patched.
Although such code works with ts-loader @artem1458 fyi
AFAIK, all ts program factory functions call createProgram
, which is where we hook in. This means it should be covered regardless of using things like incremental builder, etc.
If you're saying the resulting Program instance isn't your replaced one (and you did a persistent patch), it sounds like the plugin you're using is a different instance of typescript.
You could trace out where the plugin code is getting or requiring the typescript module it uses to for program creation. If it's required, you can use require.resolve
to find the path. There's also require cache, which could yield some information.
Given what you've shared, my biggest suspicion would be that your plugin is using an unpatched installation of typescript.
A first fast diagnostic route would be to search node modules for all installations of ts, and if you find one that your plugin is may be using, add a throw to the top of the typescript.js
file.
You might also alter the plugin code where it creates program and look for typescript.originalCreateProgram
. If that's missing, your version should be unpatched – I'm on mobile so I can't confirm, but I believe that's where we copy the old function
Although... I managed to make it work with fork-ts-checker (by turning off the 'build' option in the fork-ts-checker settings). However, the plugin behaves incorrectly. getSemanticDiagnostics is called only once, and 'sourceFile' comes as undefined. Perhaps, this is an issue with fork-ts-checker. In any case, the approach with addDiagnostic does not work with any loader and plugin (ts-loader, fork-ts-checker, rollup-plugin-typescript2). And, most likely, the method of overriding 'program.getSemanticDiagnostics' is not the most appropriate way. Although, a similar approach is used in the default TypeScript plugins.
Additionally, I'll add that ts-loader works even without the 'transformProgram': true option, using code like this:
export default function (program: Program, host: CompilerHost, pluginConfig: CliPluginConfig): TransformerPlugin {
program.getSemanticDiagnostics = (sourceFile, cancellationToken) => {
// some code
return [];
}
return () => (sourceFile) => sourceFile;
}
However, fork-ts-checker doesn't work at all with this code and behaves incorrectly with the provided one above.
I figured out with fork-ts-checker
- it indeed does not invoke afterProgramEmitAndDiagnostics
or something similar. It directly uses the 'getSemanticDiagnostics' method without parameters. It returns diagnostics for all files (similar to tspc
). However, in ts-loader
, for example, it provides diagnostics for each file separately. This is something to consider when implementing the plugin
I overrode getSemanticDiagnostics
in the transformProgram: true
mode, and everything started working. While it may seem like a workaround to me, there is a solution. Most likely, other plugins and loaders for diagnostics use similar approaches.
Sorry guys. Not sure why, but GitHub mobile app has been messing up today. I had some messages duplicate and others were deleted entirely.
It returns diagnostics for all files (similar to tspc). However, in ts-loader, for example, it provides diagnostics for each file separately.
So what we do pre-transform is hook into emitFilesAndReportErrors
. In it, we take the composite allDiagnostics
array it creates and make it available to our transformers by associating it with the Program
instance in WeakMap.
I just recalled that some of these applications have their own way of filtering diagnostics. Looks like ts-loader does indeed have that, so it's not surprising that this is the case. (options -> ignoreDiagnostics)
Something to bear in mind is that the purpose of ts-patch was to hook into tsc or the compiler API's emit process, which it does accomplish.
So, when it comes to certain cases that do their own magic with the TS compiler API, support can't be guaranteed.
However... These are popular libraries, and making sure it's supported does make sense. I already have a plan outline for better diagnostics filtering for the upcoming new major version, but maybe we can find an easy route here.
I overrode getSemanticDiagnostics in the transformProgram: true mode, and everything started working.
@DiFuks Can you give some more details on what exactly you're doing in your workaround? Also, do you know if emitFilesAndReportErrors
is ever called? It sounds like maybe ts-loader is manually calling the API to get diagnostics from Program, which would mean our patched EmitResult doesn't matter (?)
do you know if emitFilesAndReportErrors is ever called?
no :(
Thanks for sharing, @DiFuks!
Here are a couple notes that can help:
1. Use the provided ts
instance rather than import
You should use ts
from the extras: ProgramTransformerExtras
property of the signature instead of importing typescript. (In the case of a source transformer, you'd want to use its ts
provided in the extras arg)
This ensures you're using the same typescript as what was loaded initially.
2. Your Program transformer has an incorrect return
The "Program Transformer" pattern is meant to take an input Program
instance and allow you to modify it or recreate it. It should always return a Program
.
It looks like your code returns a source file transformer function pattern. In your case, you'd want to just return the original program
. No need to recreate it.
One general note. The reason I didn't add diagnostics modification functions for Program transformer is because you can do the work yourself by directly dealing with the Program instance. In your use-case, a Program transformer is definitely the correct route, and from what I can tell, your method of doing it looks reasonable. I wouldn't actually advise using a Source Transformer for your purposes.
Before you make any changes to your code to address the above, would you mind helping with something?
In your workaround code, keep everything as it is (for now), but make the following changes:
export default function (program: Program, host: CompilerHost, pluginConfig: CliPluginConfig, { ts }): TransformerPlugin {
// ...
Error.stackTraceLimit = 10000;
program.getSemanticDiagnostics = function (sourceFile, cancellationToken) {
let err = new Error(`originalCreateProgram ${ts.originalCreateProgram != null}`);
process.stderr.write(JSON.stringify(arguments, null, 2) + '\n\n');
process.stderr.write(err.stack + '\n');
process.exit(-1);
}
// ...
}
What we did was:
ts
instance in the function signature
typescript
for it at this point, though.getSemanticDiagnostics
override with one that gives us a long stacktrace + info on the ts instance (helps us determine patch status)tspc
ts-loader
setupPlease report back the outputs of each run
@nonara Thank you for your help!
Here is the report:
Also, I rewrote the plugin based on your advice. Here's the plugin code along with examples. What I found out:
yarn build:tspc # ✅ working
yarn build:ts-loader # ✅ working
yarn build:fork-ts # ✅ working
yarn watch:tspc # ⛔️not working
yarn watch:ts-loader # ✅ working
yarn watch:fork-ts # ⛔️not working
It's evident that the issues are only with tscp
and fork-ts-checker
in watch mode. In both cases, the plugin function simply isn't being called. Most likely, the problems are related. I studied the source code of fork-ts-checker
and found that in watch mode, it creates watchCompilerHost and instantiates a program through baseWatchCompilerHost.createProgram. For some reason, this method returns an unpatched program instance. I could think that the fork-ts-checker developers 'made it complicated,' but the plugin doesn't work with tscp --watch
either
I tried to come up with a drastic solution - override the createWatchCompilerHost
method in the plugin code, but unsurprisingly, I got the error message: TypeError: Cannot redefine property: createWatchCompilerHost
I would really like to bring the implementation of my plugin to completion. Since I believe it would be helpful to many people, I'm willing to assist you in fixing it (if you find it necessary)
I've made progress in finding a solution. The issue in my case is that getSemanticDiagnostics is called not from the program but from builderProgram. In turn, it calls getSemanticDiagnosticsOfFile. Only then does it call getProgramDiagnostics from the program. getProgramDiagnostics is a private method of the program. I can override it for my needs, but perhaps it will be a somewhat suboptimal workaround
The final version of the solution
For some reason, overriding getProgramDiagnostics also didn't yield results - it returned an empty array. In the end, I had to override getBindAndCheckDiagnostics. As a result, everything worked in ts-loader, tspc, forktschecker, both in build and watch modes.
@nonara @DiFuks Hey, I finally managed to make fix that should fix this problem https://github.com/nonara/ts-patch/pull/150
@DiFuks I couldn't make it works with fork-ts-checker-webpack-plugin
, even with patching getBindAndCheckDiagnostics
method, but it works with ts-loader which is good I think
@artem1458 Thank you! Oddly, the solution presented here works well with ForkTSChecker.
Unfortunately, my team specifically uses ForkTSChecker. :(
@nonara @DiFuks You know, I actually thinking about adding methods that will add different kind of diagnostics to the program
Program have a few methods for providing diagnostics So we can add methods to the TransformerExtras:
Compiler plugin developers can decide which diagnostics they want to add. @nonara What do you think about it, is it ok to add such methods? I can add them to fix it if you think that it make sense.
When using
webpack
withts-loader
andts-patch
- diagnostics produced by transformer plugin is not utilised, the result - compilation is not failing even with reported diagnostics with severity ERROR. Same issue appears when usingvite
withrollup-plugin-typescript2
andts-patch
. This issue appears in bothts-patch
modes -Live Compiler
andPersistent Patch
.When using
tsc
or ortspc
diagnostics is utilised and failing compilation as expected.Minimal reproduction repo: https://github.com/artem1458/ts-patch-webpack
Output from
build:tsc
script (failed as expected):Output from
build:webpack
script (not failing build):Output from
build:vite
script (not failing build):