Skip to content

Set maxNodeModuleJsDepth for inferred projects#11603

Merged
zhengbli merged 6 commits into
microsoft:masterfrom
zhengbli:11116
Jan 10, 2017
Merged

Set maxNodeModuleJsDepth for inferred projects#11603
zhengbli merged 6 commits into
microsoft:masterfrom
zhengbli:11116

Conversation

@zhengbli
Copy link
Copy Markdown

@zhengbli zhengbli commented Oct 13, 2016

Fixes #11116

Comment thread src/server/project.ts Outdated
this.compilerOptions.allowNonTsExtensions = true;
}

if (this.projectKind === ProjectKind.Inferred) {
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.

should we check also that the project has .js files?

@mhegazy
Copy link
Copy Markdown
Contributor

mhegazy commented Dec 22, 2016

@zhengbli can you refresh this PR.

@zhengbli zhengbli changed the title Set maxNodeModuleJsDepth for inferred projects [WIP] Set maxNodeModuleJsDepth for inferred projects Dec 28, 2016
@zhengbli zhengbli changed the title [WIP] Set maxNodeModuleJsDepth for inferred projects Set maxNodeModuleJsDepth for inferred projects Dec 28, 2016
@zhengbli
Copy link
Copy Markdown
Author

@mhegazy updated with a new implementation. Now the inferred project has a property called "isJsInferredProject" which serves as compilerOptions-independent flag so "js inferred project" can have different behavior than the normal inferred projects.

Comment thread src/server/project.ts Outdated
private _isJsInferredProject = false;
set isJsInferredProject(newValue: boolean) {
if (newValue && !this._isJsInferredProject) {
this.setCompilerOptions(this.getCompilerOptions());
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 looks odd

Copy link
Copy Markdown
Contributor

@vladima vladima Dec 28, 2016

Choose a reason for hiding this comment

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

also before calling setCompilerOptions this._isJsInferredProject is not yet set so I'm not sure how control flow should enter if statement below if this._isJsInferredProject was false

+            if (this._isJsInferredProject && typeof newOptions.maxNodeModuleJsDepth !== "number") {
 +                newOptions.maxNodeModuleJsDepth = 2;
 +            }

});
});

describe("maxNodeModuleJsDepth for inferred projects", () => {
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.

can you have a test that actually verifies the value of maxModuleDepth (i.e. that we are not looking into folders that are located deeper)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Updated the test

Comment thread src/server/project.ts Outdated
})();

private _isJsInferredProject = false;
set isJsInferredProject(newValue: boolean) {
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.

personal nit: I'm not a big fan of set-only accessors, can you turn it into method instead?

@zhengbli
Copy link
Copy Markdown
Author

zhengbli commented Jan 3, 2017

Further comments? @mhegazy @vladima

@zhengbli zhengbli merged commit 9e12796 into microsoft:master Jan 10, 2017
@zhengbli zhengbli deleted the 11116 branch January 10, 2017 20:17
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 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