Skip to content

Conversation

@vladima
Copy link
Contributor

@vladima vladima commented Apr 1, 2016

This PR continues the work started in #7549


const defaultLibrarySearchPaths = <Path[]>[
"types/",
"node_modules/",
Copy link
Member

Choose a reason for hiding this comment

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

Is it safe to remove this now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cast to Path - yes, I'll drop it. As for defaultLibrarySearchPaths - no, they are still used here

}
},
{
name: "traceModuleResolution",
Copy link
Member

Choose a reason for hiding this comment

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

Is this not a breaking change? Should you flag these as experimental?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this flag was planned to initially appear in 2.0 which was not officially released yes so no, technically it is not a breaking change. There are few folks who might already use this flag, I'll ping them once this PR is approved. Re experimental - I don't think this is applicable here.

@RyanCavanaugh
Copy link
Member

👍

return options.typesSearchPaths;
}
return options.configFilePath
? [getDirectoryPath(options.configFilePath)].concat(defaultLibrarySearchPaths)
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need the directory of the tsconfig.json?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

}
}

function getEffectiveTypesPrimarySearchPaths(options: CompilerOptions): string[] {
Copy link
Contributor

Choose a reason for hiding this comment

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

do not think we need the function here. the name is not much simpler than the actual implementation. consider inlining the function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mhegazy
Copy link
Contributor

mhegazy commented Apr 5, 2016

We should also add a new property to tsconfig.json "types" that act as /// <reference types=".." />. @RyanCavanaugh is there a reason that was not included?

@RyanCavanaugh
Copy link
Member

No, it should be included

"code": 6104
},
"Expected type of 'typings' field in 'package.json' to be 'string', got '{0}'.": {
"Expected type of '{0}' field in 'package.json' to be 'string', got '{1}'.": {
Copy link
Member

Choose a reason for hiding this comment

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

Why not generalize this to take the expected type as well?

@mhegazy
Copy link
Contributor

mhegazy commented Apr 8, 2016

👍

@vladima vladima merged commit ac6224d into master Apr 8, 2016
@vladima vladima deleted the libraryDirectives-2 branch April 8, 2016 22:45
let fileProcessingDiagnostics = createDiagnosticCollection();
let skipDefaultLib = options.noLib;
const programDiagnostics = createDiagnosticCollection();
const currentDirectory = host.getCurrentDirectory();

Choose a reason for hiding this comment

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

This line breaks the optional nature of the host argument

@jbrantly
Copy link

This PR breaks the optional nature of the host argument in createProgram.

@vladima
Copy link
Contributor Author

vladima commented Apr 11, 2016

Oops, good catch! Thanks @jbrantly for noticing this, fix will be out shortly

@microsoft microsoft locked and limited conversation to collaborators Jun 19, 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.

7 participants