Skip to content

Added deferred ScriptKind and renamed JsFileExtensionInfo to FileExte…#23191

Merged
armanio123 merged 7 commits into
microsoft:masterfrom
armanio123:AddVueSupport
May 3, 2018
Merged

Added deferred ScriptKind and renamed JsFileExtensionInfo to FileExte…#23191
armanio123 merged 7 commits into
microsoft:masterfrom
armanio123:AddVueSupport

Conversation

@armanio123
Copy link
Copy Markdown
Contributor

…nsionInfo

This change will help identifying files that the extension doesn't identify the script kind but the content does.

@armanio123 armanio123 requested a review from billti April 6, 2018 00:33
Comment thread src/compiler/core.ts Outdated
return needAllExtensions ? allSupportedExtensions : supportedTypeScriptExtensions;
export function getSupportedExtensions(options?: CompilerOptions, extraFileExtensions?: ReadonlyArray<FileExtensionInfo>): ReadonlyArray<string> {
const needJsExtensions = options && options.allowJs;
let extensions: string[] = needJsExtensions ? [...allSupportedExtensions] : [...supportedTypeScriptExtensions];
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you please not create copy of array when its not needed.

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. Although the needJsExtension comparison now repeats once.

Comment thread src/compiler/core.ts Outdated

const extensions = [
...needJsExtensions ? allSupportedExtensions : supportedTypeScriptExtensions,
...extraFileExtensions.filter(x => x.scriptKind === ScriptKind.Deferred || needJsExtensions && isJavaScriptLike(x.scriptKind)).map(x => x.extension)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

May be use
...mapDefined(extraFileExtentions, x => x.scriptKind === ScriptKind.Deferred || needJsExtensions && isJavaScriptLike(x.scriptKind) ? x.scriptKind : undefined)

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.

PriorityImpliesCombination = 28
}
interface JsFileExtensionInfo {
interface FileExtensionInfo {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this is a breaking change really. consider not changing the name, or adding a type alias type JsFileExtensionInfo = FileExtensionInfo

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.

Added type.

@sheetalkamat
Copy link
Copy Markdown
Member

Please add some test cases using the new Deffered as the script kind.

Comment thread src/compiler/types.ts
}

export interface JsFileExtensionInfo {
export type JsFileExtensionInfo = FileExtensionInfo;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

add a /** @deprecated use FileExtensionInfo instead./ comment

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.

Added.

@armanio123
Copy link
Copy Markdown
Contributor Author

Added unit test for checking that the deferred extension is included on the project context.

checkNumberOfProjects(projectService, { configuredProjects: 1 });

const configuredProject = configuredProjectAt(projectService, 0);
checkProjectActualFiles(configuredProject, [file1.path, tsconfig.path]);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Interesting.. Why is file2 not part of the project, I thought it would be expected to included in the project?

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.

File2 is a JS file and the tsconfig.json doesn't enable AllowJS. This test checks that by adding a deferred extension it gets the right context but also doesn't affect the JS setting. Hope it makes sense.

Comment thread src/compiler/core.ts Outdated

export function fileExtensionIs(path: string, extension: string): boolean {
return path.length > extension.length && endsWith(path, extension);
return path.length >= extension.length && endsWith(path, extension);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You sure you want >= here? For example a file named .ts shouldn't count.

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.

Rollback. Its no longer needed for Deferred kind.

@armanio123 armanio123 merged commit 1d593fd into microsoft:master May 3, 2018
@microsoft microsoft locked and limited conversation to collaborators Jul 31, 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.

4 participants