Skip to content

Exclude legacy safelist files in external projects #19542

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

Merged
merged 9 commits into from
Nov 10, 2017
Merged

Exclude legacy safelist files in external projects #19542

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
da63c2c
Exclude legacy safelist files in external projects
RyanCavanaugh Oct 27, 2017
5830bb9
Improved logging
RyanCavanaugh Oct 28, 2017
5395d0d
Add test
RyanCavanaugh Oct 30, 2017
5dc02ef
Use a different RegEx
RyanCavanaugh Nov 1, 2017
ddd8c95
Remove testcases we don't like
RyanCavanaugh Nov 9, 2017
19cc427
Format + new regex
RyanCavanaugh Nov 9, 2017
0e105ad
Log more usefully when this test fails
RyanCavanaugh Nov 9, 2017
b043edd
Merge
RyanCavanaugh Nov 9, 2017
0d5dec9
Remove commented tests
RyanCavanaugh Nov 9, 2017
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
7 changes: 7 additions & 0 deletions src/compiler/core.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2413,6 +2413,13 @@ namespace ts {
return <T>(removeFileExtension(path) + newExtension);
}

/**
* Takes a string like "jquery-min.4.2.3" and returns "jquery"
*/
export function removeMinAndVersionNumbers(fileName: string) {
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need this function in compiler? Can it be part of services\utilities instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

This functions needs to be used in the typings installer and in tsserver; I couldn't find another file that was shared by them. services\utilities isn't involved in the typings installer and adding it introduced more errors because it needs references to e.g. Project

return fileName.replace(/((?:\.|-)min(?=\.|$))|((?:-|\.)\d+)/g, "");
Copy link
Member

Choose a reason for hiding this comment

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

This also removes digits without the min, for example "jquery.4.2-test" also becomes "jquery-test". Probably not intended. (Also why use non-capturing groups (i.e. (?: )? You are not using the groups for anything, so it's just syntactic noise).

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 exactly should the rule be? Remove all characters after the first . if the thing after the . is min or a number?

Copy link
Member Author

Choose a reason for hiding this comment

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

@DanielRosenwasser wrote us a nice regex

}

export interface ObjectAllocator {
getNodeConstructor(): new (kind: SyntaxKind, pos?: number, end?: number) => Node;
getTokenConstructor(): new <TKind extends SyntaxKind>(kind: TKind, pos?: number, end?: number) => Token<TKind>;
Expand Down
33 changes: 27 additions & 6 deletions src/server/editorServices.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ namespace ts.server {

export interface TypesMapFile {
typesMap: SafeList;
simpleMap: string[];
simpleMap: { [libName: string]: string };
}

/**
Expand Down Expand Up @@ -374,6 +374,7 @@ namespace ts.server {

private readonly hostConfiguration: HostConfiguration;
private safelist: SafeList = defaultTypeSafeList;
private legacySafelist: { [key: string]: string } = {};

private changedFiles: ScriptInfo[];
private pendingProjectUpdates = createMap<Project>();
Expand Down Expand Up @@ -426,9 +427,12 @@ namespace ts.server {
this.toCanonicalFileName = createGetCanonicalFileName(this.host.useCaseSensitiveFileNames);
this.throttledOperations = new ThrottledOperations(this.host, this.logger);

if (opts.typesMapLocation) {
if (this.typesMapLocation) {
this.loadTypesMap();
}
else {
this.logger.info("No types map provided; using the default");
}

this.typingsInstaller.attach(this);

Expand Down Expand Up @@ -518,10 +522,12 @@ namespace ts.server {
}
// raw is now fixed and ready
this.safelist = raw.typesMap;
this.legacySafelist = raw.simpleMap;
}
catch (e) {
this.logger.info(`Error loading types map: ${e}`);
this.safelist = defaultTypeSafeList;
this.legacySafelist = {};
}
}

Expand Down Expand Up @@ -1393,7 +1399,7 @@ namespace ts.server {
return false;
}

private createExternalProject(projectFileName: string, files: protocol.ExternalFile[], options: protocol.ExternalProjectCompilerOptions, typeAcquisition: TypeAcquisition) {
private createExternalProject(projectFileName: string, files: protocol.ExternalFile[], options: protocol.ExternalProjectCompilerOptions, typeAcquisition: TypeAcquisition, excludedFiles: NormalizedPath[]) {
const compilerOptions = convertCompilerOptions(options);
const project = new ExternalProject(
projectFileName,
Expand All @@ -1402,6 +1408,7 @@ namespace ts.server {
compilerOptions,
/*languageServiceEnabled*/ !this.exceededTotalSizeLimitForNonTsFiles(projectFileName, compilerOptions, files, externalFilePropertyReader),
options.compileOnSave === undefined ? true : options.compileOnSave);
project.excludedFiles = excludedFiles;

this.addFilesToNonInferredProjectAndUpdateGraph(project, files, externalFilePropertyReader, typeAcquisition);
this.externalProjects.push(project);
Expand Down Expand Up @@ -2204,7 +2211,22 @@ namespace ts.server {
excludedFiles.push(normalizedNames[i]);
}
else {
filesToKeep.push(proj.rootFiles[i]);
let exclude = false;
if (typeAcquisition && (typeAcquisition.enable || typeAcquisition.enableAutoDiscovery)) {
const baseName = getBaseFileName(normalizedNames[i].toLowerCase());
if (fileExtensionIs(baseName, "js")) {
const inferredTypingName = removeFileExtension(baseName);
const cleanedTypingName = removeMinAndVersionNumbers(inferredTypingName);
if (this.legacySafelist[cleanedTypingName]) {
this.logger.info(`Excluded '${normalizedNames[i]}'`);
Copy link
Member

Choose a reason for hiding this comment

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

this.logger.info(Excluded '${normalizedNames[i]}'); [](start = 32, length = 53)

I think it would be helpful (and consistent) to indicate why the file is being excluded (i.e. legacy safelist).

excludedFiles.push(normalizedNames[i]);
exclude = true;
}
}
}
if (!exclude) {
filesToKeep.push(proj.rootFiles[i]);
}
}
}
proj.rootFiles = filesToKeep;
Expand Down Expand Up @@ -2312,8 +2334,7 @@ namespace ts.server {
else {
// no config files - remove the item from the collection
this.externalProjectToConfiguredProjectMap.delete(proj.projectFileName);
const newProj = this.createExternalProject(proj.projectFileName, rootFiles, proj.options, proj.typeAcquisition);
newProj.excludedFiles = excludedFiles;
this.createExternalProject(proj.projectFileName, rootFiles, proj.options, proj.typeAcquisition, excludedFiles);
}
if (!suppressRefreshOfInferredProjects) {
this.ensureProjectStructuresUptoDate(/*refreshInferredProjects*/ true);
Expand Down
2 changes: 1 addition & 1 deletion src/services/jsTyping.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ namespace ts.JsTyping {
if (!hasJavaScriptFileExtension(j)) return undefined;

const inferredTypingName = removeFileExtension(getBaseFileName(j.toLowerCase()));
const cleanedTypingName = inferredTypingName.replace(/((?:\.|-)min(?=\.|$))|((?:-|\.)\d+)/g, "");
const cleanedTypingName = removeMinAndVersionNumbers(inferredTypingName);
return safeList.get(cleanedTypingName);
});
if (fromFileNames.length) {
Expand Down
7 changes: 5 additions & 2 deletions tests/baselines/reference/api/tsserverlibrary.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7391,7 +7391,9 @@ declare namespace ts.server {
}
interface TypesMapFile {
typesMap: SafeList;
simpleMap: string[];
simpleMap: {
[libName: string]: string;
};
}
function convertFormatOptions(protocolOptions: protocol.FormatCodeSettings): FormatCodeSettings;
function convertCompilerOptions(protocolOptions: protocol.ExternalProjectCompilerOptions): CompilerOptions & protocol.CompileOnSaveMixin;
Expand Down Expand Up @@ -7468,6 +7470,7 @@ declare namespace ts.server {
private readonly throttledOperations;
private readonly hostConfiguration;
private safelist;
private legacySafelist;
private changedFiles;
private pendingProjectUpdates;
private pendingInferredProjectUpdate;
Expand Down Expand Up @@ -7576,7 +7579,7 @@ declare namespace ts.server {
private findExternalProjectByProjectName(projectFileName);
private convertConfigFileContentToProjectOptions(configFilename, cachedDirectoryStructureHost);
private exceededTotalSizeLimitForNonTsFiles<T>(name, options, fileNames, propertyReader);
private createExternalProject(projectFileName, files, options, typeAcquisition);
private createExternalProject(projectFileName, files, options, typeAcquisition, excludedFiles);
private sendProjectTelemetry(projectKey, project, projectOptions?);
private addFilesToNonInferredProjectAndUpdateGraph<T>(project, files, propertyReader, typeAcquisition);
private createConfiguredProject(configFileName);
Expand Down