WIP add useFileCompiler setting #616
Conversation
enables getting type-checking while enforcing --isolatedModules fix ivogabe#613
| @@ -0,0 +1,12 @@ | |||
| TypeScript error: [31mtest/useFileCompiler/excluded-dir/test.ts(1,1): [39merror TS2304: Cannot find name 'notCompiled'. | |||
There was a problem hiding this comment.
please ignore these tests, they are a little borked right now
|
could use some advice on this: #613 (comment) |
|
Hi Max, thanks for your PR. Sorry, I have not yet had time to look at your work, O hope to do that this evening or tomorrow. |
|
No rush, and thanks for your time |
There was a problem hiding this comment.
Finally got some time to look at your PR, I have a few suggestions to make things easier. You only changed the createProject, but I would also want to adjust the main exported function. That function is defined here:
Line 9 in d0410c2
This function also takes a reporter, which we will need to move one position to the right. I think that the GulpTsSettings are used more often than the reporters. We cannot add Reporter to the GulpTsSettings interface, as that would not work with the createProject interface; the reporter is there namely passed in the gulp task, not at the creation of the project. The signature of the main export would then thus look like this:
function compile(settings?: compile.Settings, gulpTsSettings?: GulpTsSettings, theReporter?: _reporter.Reporter): compile.CompileStream;You can remove the overload taking a project, as that API is deprecated and we can remove that in the next major release.
When we change that function as well, we assert that the typescript options is always passed in the gulpTsSettings parameter, not in the normal settings parameter. That will simplify getTypeScript as well, as that will then only take one argument. It will also simplify the documentation. Can you also make that change?
Regarding the tests, I think we get a time out as we run the test with several versions of TypeScript. We can drop some old versions and thus reduce the time to run the tests. I'll do that myself.
| if (useFileCompiler && !options.isolatedModules) { | ||
| throw Error("useFileCompiler: true can only be used if config.compilerOptions.isolatedModules is also set to true"); | ||
| } | ||
| let compiler: ICompiler; |
There was a problem hiding this comment.
This should be indented with tabs
| export function createProject(tsConfigFileName: string, settings?: Settings): Project; | ||
| export function createProject(settings?: Settings): Project; | ||
| export function createProject(fileNameOrSettings?: string | Settings, settings?: Settings): Project { | ||
| function _createProject({ |
There was a problem hiding this comment.
Normal arguments instead of an object, as you are giving all properties at both calls. So an object does not really add a value here.
|
closing, will reopen after incorporating changes |
|
Hi Max, have you had time to look into this? Or shall I proceed with this work? |
|
Apologies, I had to put this aside as we switched to a different solution. I can tidy this up next Sunday, but I understand if you would prefer to take over from here: whatever works. Thanks again for your feedback! |
|
No problem, I also had some other things to do. No hurries, would be great if you could make some time next Sunday! |
|
Hi @ivogabe , I'm sorry, but I think I'll have to orphan this PR. It started with a single new flag, but triggered a large refactor of the public API that I'm having trouble fitting in my head, given all the overloads and optional arguments. I'm no longer using gulp-typescript, so don't want to be a bad user that does a drive-by PR that has such cascading affects on tests, docs, and the future maintainability of the project. Thanks for your work on gulp-typescript, and apologies for bailing! |
enables getting type-checking
while enforcing --isolatedModules
fix
#613