Added support for TypeScript declaration map #821
Conversation
|
First of all, thanks for the PR! Question; is there overlap with the work @andrewbranch is doing in this PR? He's been busy beavering away for the last week or so and it seems that there might be commonality here. @andrewbranch can you comment on the work @JonWallsten has done. I'm wondering if it make sense for you guys to collaborate on this rather than having related PRs merged separately. |
|
@johnnyreilly I was actually checking out his PR earlier today. I don't think they collide. He should be able to merge this without conflicts. My changes are minor as you can see. I only allowed the regex to include .d.ts.map as well and made sure that the provideDeclarationFilesToWebpack function handles multiple output files (d.ts and d.ts.map). We should be able to merge this before his. And I guess project references and declarationMap are totally unrelated. They just happens to be new features in 3.0.x. |
src/instances.ts
Outdated
| @@ -205,11 +205,11 @@ function successfulTypeScriptInstance( | |||
| let normalizedFilePath: string; | |||
| try { | |||
| const filesToLoad = loaderOptions.onlyCompileBundledFiles | |||
| ? configParseResult.fileNames.filter(fileName => | |||
| ? configParseResult.fileNames.filter((fileName: string) => | |||
There was a problem hiding this comment.
is the type annotation necessary? Presumably the type can be inferred?
There was a problem hiding this comment.
I got an error in VS code, that's why I fixed it. But now when I check the tsconfig I can't find "noImplicitAny", so I'm not actually sure why I got that warning. VSCode has been a little shaky when it comes to errors since last release though. I don't have a problem removing them.
src/instances.ts
Outdated
| dtsDtsxRegex.test(fileName) | ||
| ) | ||
| : configParseResult.fileNames; | ||
| filesToLoad.forEach(filePath => { | ||
| filesToLoad.forEach((filePath: string) => { |
There was a problem hiding this comment.
is the type annotation necessary? Presumably the type can be inferred?
Me neither 😄 |
src/after-compile.ts
Outdated
| size: () => declarationFile.text.length | ||
| }; | ||
| const declarationFiles: typescript.OutputFile[] = outputFiles | ||
| .filter((outputFile: typescript.OutputFile) => outputFile.name.match(constants.dtsDtsxRegex)); |
There was a problem hiding this comment.
can (outputFile: typescript.OutputFile) just be outputFile? Again type inference should work
There was a problem hiding this comment.
Yeah, you are correct. I reinstalled VS Code, and it works now. I'm removing these.
src/after-compile.ts
Outdated
| .filter((outputFile: typescript.OutputFile) => outputFile.name.match(constants.dtsDtsxRegex)); | ||
|
|
||
| if (Array.isArray(declarationFiles)) { | ||
| declarationFiles.forEach(file => { |
There was a problem hiding this comment.
can file be declarationFile please?
src/constants.ts
Outdated
| @@ -12,7 +12,7 @@ export const ScriptTargetES2015 = 2; | |||
| export const ModuleKindCommonJs = 1; | |||
|
|
|||
| export const tsTsxRegex = /\.ts(x?)$/i; | |||
| export const dtsDtsxRegex = /\.d\.ts(x?)$/i; | |||
| export const dtsDtsxRegex = /\.d\.ts(x?)(\.map)?$/i; | |||
There was a problem hiding this comment.
Can this be dtsDtsxRegex be dtsDtsxOrDtsDtsxMapRegex please? It's verbose but hopefully clear.
There was a problem hiding this comment.
Absolutely! I'll fix!
src/after-compile.ts
Outdated
| const declarationFiles: typescript.OutputFile[] = outputFiles | ||
| .filter((outputFile: typescript.OutputFile) => outputFile.name.match(constants.dtsDtsxRegex)); | ||
|
|
||
| if (Array.isArray(declarationFiles)) { |
There was a problem hiding this comment.
I wonder if you could tell me about this part some more? This is the crux of the PR really; previously the code assumed a single d.ts file. Now it's multiple; presumably that's allowing for the map files as well?
There was a problem hiding this comment.
Yes, exactly! Due to the regex change, getEmitOutput will return both d.ts and d.ts.map when declarationMap is true. It felt logical to handle declaration & map files together since they are linked together inside the d.ts file, and besides the map file can't live without the declaration file. When it comes to performance loss for using a forEach instead of a pop() when it's only one should not a problem since we're not using a million and millions of files (i hope).
src/after-compile.ts
Outdated
| outputFile.name.match(constants.dtsDtsxOrDtsDtsxMapRegex) | ||
| ); | ||
|
|
||
| if (Array.isArray(declarationFiles)) { |
There was a problem hiding this comment.
Is Array.isArray the right thing to do here? Presumably this will always be true as the result of the filter will always be an Array?
There was a problem hiding this comment.
Yeah, that is correct. But forEach will handle that. I'm only doing the array check to make sure forEach won't throw and error.
Edit: I see your point now. The original line was declarationFiles !== undefined. So I assumed (maybe wrongly) that I could also be undefined, even though I was surprised about that.
There was a problem hiding this comment.
Filter will always return an array. So I removed the check. It's safe to assume that we will always have an array, but it might be empty.
Removed unnecessary typings Removed unnecessary array check
|
Looks good. I'll plan to merge and ship this shortly; thanks for your work! |
|
Perfect! Pleasure's all mine. I really want the feature. I'm glad it was easy to implement. Good work of the code base. |
@johnnyreilly: Please check this out and let me know if you want any changes or if I did something wrong creating the tests.
Fix for #820