Skip to content

Conversation

alan-agius4
Copy link
Contributor

@alan-agius4 alan-agius4 commented Mar 31, 2018

Would love to add some Unit Tests but kinda can't find a way where to test it. If any one is willing to give me pointers on how do you guy usually test this. I'll happily add them

Ignore falsy file names from getSourceFileFromReferenceWorker

Closes: #13629

@alan-agius4 alan-agius4 force-pushed the feature/compiler-host-falsy-file branch 2 times, most recently from ab6ac08 to e914821 Compare March 31, 2018 16:14
@mhegazy
Copy link
Contributor

mhegazy commented Mar 31, 2018

@alan-agius4 we will need a unit test to go with this change. i would say you need a new file, since we do not have any existing unit test files that cover this area.
you can look at https://github.com/Microsoft/TypeScript/blob/master/src/harness/unittests/hostNewLineSupport.ts for reference for testing compilerHost use.

@alan-agius4
Copy link
Contributor Author

@mhegazy, thanks for the example, that was helpful.

I added the some tests.

@@ -1703,7 +1703,11 @@ namespace ts {
fail?: (diagnostic: DiagnosticMessage, ...argument: string[]) => void,
refFile?: SourceFile): SourceFile | undefined {

if (hasExtension(fileName)) {
// Don't process falsy fileNames
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about doing this check on the call to getDefaultLibraryFileName instead. it is making me a bit nervous to have it in this general utility function, just to handle the return of getDefaultLibraryFileName.

Copy link
Contributor Author

@alan-agius4 alan-agius4 Apr 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making you nervous is not a good sign.

You mean here: https://github.com/alan-agius4/TypeScript/blob/a27cd8f9a2e83d4b6fbf4e77c5041d94b01d86f6/src/compiler/program.ts#L611

Change it to something like

   const defaultLibraryFileName = getDefaultLibraryFileName();
                if (!options.lib && defaultLibraryFileName) {
                    processRootFile(defaultLibraryFileName, /*isDefaultLib*/ true);
                }

Ps: I make the change already, and if you are not too convinced about this. I don't mind closing the PR, just leave a comment in the original issue :)

…esides the one being compiled

Ignore falsy file names from `getDefaultLibraryFileName`

Closes: microsoft#13629
@alan-agius4 alan-agius4 force-pushed the feature/compiler-host-falsy-file branch from a27cd8f to 7e482b2 Compare April 2, 2018 18:58
@mhegazy
Copy link
Contributor

mhegazy commented Apr 3, 2018

thanks. this seems much targeted now. sorry for the back and forth.

@mhegazy mhegazy merged commit e6fa4e4 into microsoft:master Apr 3, 2018
@alan-agius4 alan-agius4 deleted the feature/compiler-host-falsy-file branch April 3, 2018 05:11
@microsoft microsoft locked and limited conversation to collaborators Jul 25, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants