Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

fix: CompilerHost.getSourceFile is being called for odd filenames#23043

Merged
mhegazy merged 1 commit into
microsoft:mastermicrosoft/TypeScript:masterfrom
alan-agius4:feature/compiler-host-falsy-filealan-agius4/TypeScript:feature/compiler-host-falsy-fileCopy head branch name to clipboard
Apr 3, 2018
Merged

fix: CompilerHost.getSourceFile is being called for odd filenames#23043
mhegazy merged 1 commit into
microsoft:mastermicrosoft/TypeScript:masterfrom
alan-agius4:feature/compiler-host-falsy-filealan-agius4/TypeScript:feature/compiler-host-falsy-fileCopy head branch name to clipboard

Conversation

@alan-agius4

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

Copy link
Copy Markdown
Contributor

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

mhegazy commented Mar 31, 2018

Copy link
Copy Markdown
Contributor

@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
Copy Markdown
Contributor Author

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

I added the some tests.

Comment thread src/compiler/program.ts Outdated
refFile?: SourceFile): SourceFile | undefined {

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

Copy link
Copy Markdown
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.

@alan-agius4 alan-agius4 Apr 2, 2018

Copy link
Copy Markdown
Contributor Author

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

mhegazy commented Apr 3, 2018

Copy link
Copy Markdown
Contributor

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

Morty Proxy This is a proxified and sanitized view of the page, visit original site.