Skip to content

At types prefix and import suffix#15545

Merged
aozgaa merged 9 commits into
microsoft:masterfrom
aozgaa:atTypesPrefixAndImportSuffix
May 22, 2017
Merged

At types prefix and import suffix#15545
aozgaa merged 9 commits into
microsoft:masterfrom
aozgaa:atTypesPrefixAndImportSuffix

Conversation

@aozgaa
Copy link
Copy Markdown
Contributor

@aozgaa aozgaa commented May 3, 2017

Fixes #14157

@aozgaa aozgaa requested review from a user, DanielRosenwasser and billti May 3, 2017 00:26
Comment thread src/services/codefixes/importFixes.ts Outdated
}

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

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
Copy Markdown
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
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

changed.

Comment thread src/services/codefixes/importFixes.ts Outdated
relativeFileName = getRelativePath(moduleFileName, sourceDirectory);
}

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

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
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

changed.

Comment thread src/compiler/core.ts Outdated
/* @internal */
export function startsWith(str: string, prefix: string): boolean {
return str.lastIndexOf(prefix, 0) === 0;
return str.indexOf(prefix) === 0;
Copy link
Copy Markdown

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.

Comment thread src/compiler/core.ts Outdated
}

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

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
Copy Markdown
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
Copy Markdown
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.

Comment thread src/compiler/core.ts Outdated
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
Copy Markdown

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.

Comment thread src/services/codefixes/importFixes.ts Outdated
catch (e) { }
}

relativeFileName = getPackageNameFromAtTypesDirectory(relativeFileName);
Copy link
Copy Markdown

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.

Comment thread src/compiler/core.ts
}

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

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.

return moduleName;
}

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

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
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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