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.

Sign up for GitHub

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

Implicitly consider an extensionless file in "includes" to be a recursive directory glob #11495

Merged
5 commits merged into from
Oct 31, 2016
Merged

Implicitly consider an extensionless file in "includes" to be a recursive directory glob #11495

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2f77a54
Implicitly consider an extensionless file in "includes" to be a recur…
Oct 7, 2016
9b2e6a3
Respond to PR comments
Oct 12, 2016
7157692
Respond to PR comments
Oct 24, 2016
1b15646
Simplify isImplicitGlob test
Oct 25, 2016
040942f
Merge branch 'master' into includes_glob
Oct 25, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 19 additions & 9 deletions src/compiler/commandLineParser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -984,9 +984,7 @@ namespace ts {
function convertTypingOptionsFromJsonWorker(jsonOptions: any,
basePath: string, errors: Diagnostic[], configFileName?: string): TypingOptions {

const options: TypingOptions = getBaseFileName(configFileName) === "jsconfig.json"
? { enableAutoDiscovery: true, include: [], exclude: [] }
: { enableAutoDiscovery: false, include: [], exclude: [] };
const options: TypingOptions = { enableAutoDiscovery: getBaseFileName(configFileName) === "jsconfig.json", include: [], exclude: [] };
convertOptionsFromJson(typingOptionDeclarations, jsonOptions, basePath, options, Diagnostics.Unknown_typing_option_0, errors);
return options;
}
Expand Down Expand Up @@ -1239,12 +1237,13 @@ namespace ts {
/**
* Gets directories in a set of include patterns that should be watched for changes.
*/
function getWildcardDirectories(include: string[], exclude: string[], path: string, useCaseSensitiveFileNames: boolean) {
function getWildcardDirectories(include: string[], exclude: string[], path: string, useCaseSensitiveFileNames: boolean): Map<WatchDirectoryFlags> {
// We watch a directory recursively if it contains a wildcard anywhere in a directory segment
// of the pattern:
//
// /a/b/**/d - Watch /a/b recursively to catch changes to any d in any subfolder recursively
// /a/b/*/d - Watch /a/b recursively to catch any d in any immediate subfolder, even if a new subfolder is added
// /a/b - Watch /a/b recursively to catch changes to anything in any recursive subfoler
//
// We watch a directory without recursion if it contains a wildcard in the file segment of
// the pattern:
Expand All @@ -1257,15 +1256,14 @@ namespace ts {
if (include !== undefined) {
const recursiveKeys: string[] = [];
for (const file of include) {
const name = normalizePath(combinePaths(path, file));
if (excludeRegex && excludeRegex.test(name)) {
const spec = normalizePath(combinePaths(path, file));
if (excludeRegex && excludeRegex.test(spec)) {
continue;
}

const match = wildcardDirectoryPattern.exec(name);
const match = getWildcardDirectoryFromSpec(spec, useCaseSensitiveFileNames);
if (match) {
const key = useCaseSensitiveFileNames ? match[0] : match[0].toLowerCase();
const flags = watchRecursivePattern.test(name) ? WatchDirectoryFlags.Recursive : WatchDirectoryFlags.None;
const { key, flags } = match;
const existingFlags = wildcardDirectories[key];
if (existingFlags === undefined || existingFlags < flags) {
wildcardDirectories[key] = flags;
Expand All @@ -1289,6 +1287,18 @@ namespace ts {
return wildcardDirectories;
}

function getWildcardDirectoryFromSpec(spec: string, useCaseSensitiveFileNames: boolean): { key: string, flags: WatchDirectoryFlags } | undefined {
const match = wildcardDirectoryPattern.exec(spec);
return match
Copy link
Member

@DanielRosenwasser DanielRosenwasser Oct 11, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

can you turn this into a series of ifs? this is pretty unreadable to me.

? {
key: useCaseSensitiveFileNames ? match[0] : match[0].toLowerCase(),
flags: watchRecursivePattern.test(spec) ? WatchDirectoryFlags.Recursive : WatchDirectoryFlags.None
}
: isImplicitGlob(spec)
? { key: spec, flags: WatchDirectoryFlags.Recursive }
: undefined;
}

/**
* Determines whether a literal or wildcard file has already been included that has a higher
* extension priority.
Expand Down
173 changes: 102 additions & 71 deletions src/compiler/core.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1177,7 +1177,7 @@ namespace ts {

/**
* Returns the path except for its basename. Eg:
*
*
* /path/to/file.ext -> /path/to
*/
export function getDirectoryPath(path: Path): Path;
Expand All @@ -1186,6 +1186,12 @@ namespace ts {
return path.substr(0, Math.max(getRootLength(path), path.lastIndexOf(directorySeparator)));
}

function getBasename(path: Path): Path;
function getBasename(path: string): string;
function getBasename(path: string): any {
Copy link
Member

Choose a reason for hiding this comment

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

You shouldn't need to use any - our overload compatibility rules are bidirectional for return types.

return path.substr(Math.max(getRootLength(path), path.lastIndexOf(directorySeparator)));
}

export function isUrl(path: string) {
return path && !isRootedDiskPath(path) && path.indexOf("://") !== -1;
}
Expand Down Expand Up @@ -1476,7 +1482,7 @@ namespace ts {
return undefined;
}

const replaceWildcardCharacter = usage === "files" ? replaceWildCardCharacterFiles : replaceWildCardCharacterOther;
const replaceWildcardCharacter = usage === "files" ? replaceWildCardCharacterFiles : replaceWildCardCharacterOther;
const singleAsteriskRegexFragment = usage === "files" ? singleAsteriskRegexFragmentFiles : singleAsteriskRegexFragmentOther;

/**
Expand All @@ -1487,81 +1493,103 @@ namespace ts {

let pattern = "";
let hasWrittenSubpattern = false;
spec: for (const spec of specs) {
for (const spec of specs) {
if (!spec) {
continue;
}

let subpattern = "";
let hasRecursiveDirectoryWildcard = false;
let hasWrittenComponent = false;
const components = getNormalizedPathComponents(spec, basePath);
if (usage !== "exclude" && components[components.length - 1] === "**") {
continue spec;
const subPattern = getSubPatternFromSpec(spec, basePath, usage, singleAsteriskRegexFragment, doubleAsteriskRegexFragment, replaceWildcardCharacter);
if (subPattern === undefined) {
continue;
}

// getNormalizedPathComponents includes the separator for the root component.
// We need to remove to create our regex correctly.
components[0] = removeTrailingDirectorySeparator(components[0]);
if (hasWrittenSubpattern) {
pattern += "|";
}

let optionalCount = 0;
for (let component of components) {
if (component === "**") {
if (hasRecursiveDirectoryWildcard) {
continue spec;
}
pattern += "(" + subPattern + ")";
hasWrittenSubpattern = true;
}

subpattern += doubleAsteriskRegexFragment;
hasRecursiveDirectoryWildcard = true;
hasWrittenComponent = true;
}
else {
if (usage === "directories") {
subpattern += "(";
optionalCount++;
}
if (!pattern) {
return undefined;
}

if (hasWrittenComponent) {
subpattern += directorySeparator;
}
return "^(" + pattern + (usage === "exclude" ? ")($|/)" : ")$");
Copy link
Member

Choose a reason for hiding this comment

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

Separate the last part into a variable and comment what the purpose is.

}

if (usage !== "exclude") {
// The * and ? wildcards should not match directories or files that start with . if they
// appear first in a component. Dotted directories and files can be included explicitly
// like so: **/.*/.*
if (component.charCodeAt(0) === CharacterCodes.asterisk) {
subpattern += "([^./]" + singleAsteriskRegexFragment + ")?";
component = component.substr(1);
}
else if (component.charCodeAt(0) === CharacterCodes.question) {
subpattern += "[^./]";
component = component.substr(1);
}
}
/**
* An "includes" path "foo" is implicitly a glob "foo\**\*" (replace \ with /) if its last component has no extension,
Copy link
Member

Choose a reason for hiding this comment

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

what do you mean by "replace \ with /"?

* and does not contain any glob characters itself.
*/
export function isImplicitGlob(lastPathComponent: string): boolean {
return !/[.*?]/.test(lastPathComponent);
}

function getSubPatternFromSpec(spec: string, basePath: string, usage: "files" | "directories" | "exclude", singleAsteriskRegexFragment: string, doubleAsteriskRegexFragment: string, replaceWildcardCharacter: (match: string) => string): string | undefined {
let subpattern = "";
let hasRecursiveDirectoryWildcard = false;
let hasWrittenComponent = false;
const components = getNormalizedPathComponents(spec, basePath);
const lastComponent = lastOrUndefined(components);
if (usage !== "exclude" && lastComponent === "**") {
return undefined;
}

// getNormalizedPathComponents includes the separator for the root component.
// We need to remove to create our regex correctly.
components[0] = removeTrailingDirectorySeparator(components[0]);

subpattern += component.replace(reservedCharacterPattern, replaceWildcardCharacter);
hasWrittenComponent = true;
if (isImplicitGlob(lastComponent)) {
components.push("**", "*");
}

let optionalCount = 0;
for (let component of components) {
if (component === "**") {
if (hasRecursiveDirectoryWildcard) {
return undefined;
}
}

while (optionalCount > 0) {
subpattern += ")?";
optionalCount--;
subpattern += doubleAsteriskRegexFragment;
hasRecursiveDirectoryWildcard = true;
}
else {
if (usage === "directories") {
subpattern += "(";
optionalCount++;
}

if (hasWrittenSubpattern) {
pattern += "|";
if (hasWrittenComponent) {
subpattern += directorySeparator;
}

if (usage !== "exclude") {
// The * and ? wildcards should not match directories or files that start with . if they
// appear first in a component. Dotted directories and files can be included explicitly
// like so: **/.*/.*
if (component.charCodeAt(0) === CharacterCodes.asterisk) {
subpattern += "([^./]" + singleAsteriskRegexFragment + ")?";
component = component.substr(1);
}
else if (component.charCodeAt(0) === CharacterCodes.question) {
subpattern += "[^./]";
component = component.substr(1);
}
}

subpattern += component.replace(reservedCharacterPattern, replaceWildcardCharacter);
}

pattern += "(" + subpattern + ")";
hasWrittenSubpattern = true;
hasWrittenComponent = true;
}

if (!pattern) {
return undefined;
while (optionalCount > 0) {
subpattern += ")?";
optionalCount--;
}

return "^(" + pattern + (usage === "exclude" ? ")($|/)" : ")$");
return subpattern;
}

function replaceWildCardCharacterFiles(match: string) {
Expand Down Expand Up @@ -1648,43 +1676,46 @@ namespace ts {
function getBasePaths(path: string, includes: string[], useCaseSensitiveFileNames: boolean) {
// Storage for our results in the form of literal paths (e.g. the paths as written by the user).
const basePaths: string[] = [path];

if (includes) {
// Storage for literal base paths amongst the include patterns.
const includeBasePaths: string[] = [];
for (const include of includes) {
// We also need to check the relative paths by converting them to absolute and normalizing
// in case they escape the base path (e.g "..\somedirectory")
const absolute: string = isRootedDiskPath(include) ? include : normalizePath(combinePaths(path, include));

const wildcardOffset = indexOfAnyCharCode(absolute, wildcardCharCodes);
const includeBasePath = wildcardOffset < 0
? removeTrailingDirectorySeparator(getDirectoryPath(absolute))
: absolute.substring(0, absolute.lastIndexOf(directorySeparator, wildcardOffset));

// Append the literal and canonical candidate base paths.
includeBasePaths.push(includeBasePath);
includeBasePaths.push(getIncludeBasePath(absolute));
}

// Sort the offsets array using either the literal or canonical path representations.
includeBasePaths.sort(useCaseSensitiveFileNames ? compareStrings : compareStringsCaseInsensitive);

// Iterate over each include base path and include unique base paths that are not a
// subpath of an existing base path
include: for (let i = 0; i < includeBasePaths.length; i++) {
const includeBasePath = includeBasePaths[i];
for (let j = 0; j < basePaths.length; j++) {
if (containsPath(basePaths[j], includeBasePath, path, !useCaseSensitiveFileNames)) {
continue include;
}
for (const includeBasePath of includeBasePaths) {
if (ts.every(basePaths, basePath => !containsPath(basePath, includeBasePath, path, !useCaseSensitiveFileNames))) {
basePaths.push(includeBasePath);
}

basePaths.push(includeBasePath);
}
}

return basePaths;
}

function getIncludeBasePath(absolute: string): string {
const wildcardOffset = indexOfAnyCharCode(absolute, wildcardCharCodes);
if (wildcardOffset < 0) {
// No "*" in the path
Copy link
Member

Choose a reason for hiding this comment

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

Isn't isImplicitGlob redoing the work of the call to indexOfAnyCharCode above? We have already eliminated * and ? as possible characters, so the only character you really need to test for is ..

Also the comment should probably read // No "*" or "?" in the path

return isImplicitGlob(getBasename(absolute))
? absolute
: removeTrailingDirectorySeparator(getDirectoryPath(absolute));
}
else {
return absolute.substring(0, absolute.lastIndexOf(directorySeparator, wildcardOffset));
}
Copy link
Member

Choose a reason for hiding this comment

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

Same nit as above about if-elses

}

export function ensureScriptKind(fileName: string, scriptKind?: ScriptKind): ScriptKind {
// Using scriptKind as a condition handles both:
// - 'scriptKind' is unspecified and thus it is `undefined`
Expand Down
1 change: 1 addition & 0 deletions src/compiler/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3073,6 +3073,7 @@ namespace ts {
Pretty,
}

/** Either a parsed command line or a parsed tsconfig.json */
export interface ParsedCommandLine {
options: CompilerOptions;
typingOptions?: TypingOptions;
Expand Down
Loading