Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Automatically consume @types/ folders #8670

Merged
merged 12 commits into from
May 24, 2016

Conversation

RyanCavanaugh
Copy link
Member

@RyanCavanaugh RyanCavanaugh commented May 18, 2016

Fixes #8275

C++-side implementation of getDirctories is in internal PR 22662

@RyanCavanaugh
Copy link
Member Author

@vladima any ideas why the baselines are failing on the CI server but not locally?

@DanielRosenwasser
Copy link
Member

It could potentially have to do with with case-sensitivity. The CI servers run Linux.

@RyanCavanaugh
Copy link
Member Author

Anyone? This is blocking the blog post

for (let i = 0; i < options.types.length; i++) {
processTypeReferenceDirective(options.types[i], resolutions[i]);
let typeReferences: string[];
if (options.types) {
Copy link
Contributor

Choose a reason for hiding this comment

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

it won't work in VS since these files won't be discovered during preprocessing (both then and else statements are affected)

Copy link
Member Author

Choose a reason for hiding this comment

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

What do we need to do?

@@ -402,6 +410,10 @@ namespace ts {
return fileSystemEntryExists(path, FileSystemEntryKind.Directory);
}

function getDirectories(path: string): string[] {
return _fs.readdirSync(path);
Copy link
Contributor

Choose a reason for hiding this comment

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

now it returns both files and folders. is it the intent?

Copy link
Member Author

@RyanCavanaugh RyanCavanaugh May 23, 2016

Choose a reason for hiding this comment

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

Fixed


// or load all types from the automatic type import fields
if (host && host.getDefaultTypeDirectiveNames) {
const commonRoot = computeCommonSourceDirectoryOfFilenames(rootFiles, host.getCurrentDirectory(), host.getCanonicalFileName);
Copy link
Contributor

Choose a reason for hiding this comment

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

currently we are tearing off this which might be wrong in cases if custom version of CompilerHost uses it in its implementation of getCanonicalFileName. Probably instead we can make computeCommonSourceDirectoryOfFilenames to accept { getCanonicalFileName(s: string): string } so we can just pass host/

Copy link
Member Author

Choose a reason for hiding this comment

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

It also needs getDefaultTypeDirectiveNames

if (typeReferences) {
const resolutions = resolveTypeReferenceDirectiveNamesWorker(typeReferences, /*containingFile*/ undefined);
for (let i = 0; i < typeReferences.length; i++) {
processTypeReferenceDirective(typeReferences[i], resolutions[i]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just add types and node_modules\@types in parseJsonConfigFileContent.getFileNames?

Copy link
Member Author

Choose a reason for hiding this comment

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

Is that called even if there's no config file?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, but why do we need to do this if there is no confit file?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we want this to be useful even if you're specifying options on the commandline (or maybe I'm misunderstanding what you're getting at?)

@RyanCavanaugh
Copy link
Member Author

Any other comments?

@RyanCavanaugh RyanCavanaugh merged commit 7173fa8 into microsoft:master May 24, 2016
@@ -161,6 +163,11 @@ namespace ts {
return result.sort();
}

function getDirectories(path: string): string[] {
const folder = fso.GetFolder(path);
return getNames(folder.subfolders);
Copy link
Contributor

Choose a reason for hiding this comment

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

SubFolders is the canonical spelling

@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.

6 participants