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

At types prefix and import suffix #15545

Merged
merged 9 commits into from
May 22, 2017

Conversation

aozgaa
Copy link
Contributor

@aozgaa aozgaa commented May 3, 2017

Fixes #14157

@aozgaa aozgaa requested review from DanielRosenwasser, billti and a user May 3, 2017 00:26
@@ -501,6 +501,14 @@ namespace ts.codefix {
relativeFileName = getRelativePath(moduleFileName, sourceDirectory);
}

if (startsWith(relativeFileName, "@types/")) {
relativeFileName = relativeFileName.substr(/*"@types".length*/ 7);
Copy link

Choose a reason for hiding this comment

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

"@types".length is 6. Why not just use that expression? You could even add a stripStart(str, prefix): string | undefined helper so you don't have to repeat this kind of code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be "@types/"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed.

@@ -501,6 +501,14 @@ namespace ts.codefix {
relativeFileName = getRelativePath(moduleFileName, sourceDirectory);
}

if (startsWith(relativeFileName, "@types/")) {
Copy link

Choose a reason for hiding this comment

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

It would be nice to have this code next to mangleScopedPackage in moduleNameResolver.ts. Maybe call it getPackageNameFromAtTypesDirectory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed.

@@ -1730,13 +1730,18 @@ namespace ts {

/* @internal */
export function startsWith(str: string, prefix: string): boolean {
return str.lastIndexOf(prefix, 0) === 0;
return str.indexOf(prefix) === 0;
Copy link

Choose a reason for hiding this comment

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

This will change the performance -- previously it would only look at the start, now it will look everywhere and then check whether the result was the start.

}

/* @internal */
export function removePrefix(str: string, prefix: string | undefined): string {
Copy link

Choose a reason for hiding this comment

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

Why allow an undefined prefix? This calls startsWith which demands a defined prefix.
We can't get --strictNullChecks on soon enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

startsWith works with an undefined prefix. I'll change the annotation there as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nevermind, it's unnecessary.

}

/* @internal */
export function endsWith(str: string, suffix: string): boolean {
const expectedPos = str.length - suffix.length;
return expectedPos >= 0 && str.indexOf(suffix, expectedPos) === expectedPos;
return expectedPos >= 0 && str.lastIndexOf(suffix, expectedPos) === expectedPos;
Copy link

Choose a reason for hiding this comment

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

See my comment on startsWith.

@@ -522,6 +522,8 @@ namespace ts.codefix {
catch (e) { }
}

relativeFileName = getPackageNameFromAtTypesDirectory(relativeFileName);
Copy link

Choose a reason for hiding this comment

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

Don't need an assignment statement if you're just returning that expression.

@@ -1747,7 +1752,7 @@ namespace ts {
return path.length > extension.length && endsWith(path, extension);
}

export function fileExtensionIsAny(path: string, extensions: string[]): boolean {
export function fileExtensionIsOneOf(path: string, extensions: string[]): boolean {
Copy link

Choose a reason for hiding this comment

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

This should probably be @internal too.

@@ -973,6 +976,16 @@ namespace ts {
return moduleName;
}

export function getPackageNameFromAtTypesDirectory(mangledName: string): string {
Copy link

Choose a reason for hiding this comment

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

@internal


// @Filename: node_modules/@types/myLib/index.d.ts
//// export function f1() {}
//// export var v1 = 5;
Copy link

Choose a reason for hiding this comment

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

This doesn't seem to be used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It exercises whether we look through the set of exported members correctly.

@mhegazy
Copy link
Contributor

mhegazy commented May 22, 2017

@aozgaa is this PR ready to go?

@aozgaa aozgaa merged commit 567b10d into microsoft:master May 22, 2017
@microsoft microsoft locked and limited conversation to collaborators Jun 14, 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.

3 participants