Skip to content

findAllReferences/rename: Search in all open projects#25648

Merged
4 commits merged into
masterfrom
findAllRefs_upstream
Jul 20, 2018
Merged

findAllReferences/rename: Search in all open projects#25648
4 commits merged into
masterfrom
findAllRefs_upstream

Conversation

@ghost
Copy link
Copy Markdown

@ghost ghost commented Jul 13, 2018

Previously if a project A depended on a project B, we would find references in both only if the reference request started at A. Now the request can start in B, and we will find references in A as long project A has been opened.

@ghost ghost requested review from RyanCavanaugh and sheetalkamat July 13, 2018 22:14
@ghost ghost force-pushed the findAllRefs_upstream branch from 366568c to 0265fff Compare July 13, 2018 22:47
Comment thread src/compiler/program.ts Outdated
getCommonSourceDirectory,
emit,
getDeclarationEmitPath: fileName => {
const file = getSourceFile(fileName);
Copy link
Copy Markdown
Contributor

@mhegazy mhegazy Jul 16, 2018

Choose a reason for hiding this comment

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

why does this need to be on the program? getDeclarationEmitOutputFilePath is already available outside this closure?

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.

getEmitHost() isn't though. That function appears to need host.getCompilerOptions, host.getCurrentDirectory, host.getCommonSourceDirectory, and host.getCanonicalFileName. getCurrentDirectory and getCanonicalFileName can be answered without a Program, but I don't think the other two can.

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.

getCurrentDirectory and getCanonicalFileName should be there on project and can be answered from there.

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.

That still leaves getCompilerOptions and getCommonSourceDirectory unfortunately.

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.

getCompilerOptions is also present on project and getCommonSourceDirectory can be added as a private method to the project that redirects to the program ?

Comment thread src/server/session.ts Outdated
combineProjectOutputWorker(
projects,
defaultProject,
{ fileName: "<arbitrary>", position: 0 }, // Location is ignored by callback
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.

Why cant you pass null instead ? If location is ignored in combineProjectOutputWorker Dont think you should have need to create default value just for passing sake.

Comment thread src/server/session.ts
projectAndLocation: ProjectAndLocation,
function getDefinitionInProject(definition: sourcemaps.SourceMappableLocation | undefined, definingProject: Project, project: Project): sourcemaps.SourceMappableLocation | undefined {
if (!definition || project.containsFile(toNormalizedPath(definition.fileName))) return definition;
const mappedDefinition = definingProject.getLanguageService().getSourceMapper().tryGetGeneratedLocation(definition);
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 also want to check if definingProject.languageServiceEnabled

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.

That comes from this.getDefaultProject, shouldn't it always be enabled?

@ghost ghost force-pushed the findAllRefs_upstream branch from b8f9274 to bd64c07 Compare July 19, 2018 23:24
@ghost
Copy link
Copy Markdown
Author

ghost commented Jul 20, 2018

@sheetalkamat Please re-review

@mhegazy
Copy link
Copy Markdown
Contributor

mhegazy commented Jul 20, 2018

@Andy-MS please port to release-3.0 as well

@ghost ghost merged commit 93722c8 into master Jul 20, 2018
@ghost ghost deleted the findAllRefs_upstream branch July 20, 2018 23:54
ghost pushed a commit that referenced this pull request Jul 20, 2018
* findAllReferences/rename: Search in all open projects

* Avoid needing a dummy location when location is unused

* Remove Program#getDeclarationEmitPath

* Only iterate over enabled projects
ghost pushed a commit that referenced this pull request Jul 21, 2018
* findAllReferences/rename: Search in all open projects

* Avoid needing a dummy location when location is unused

* Remove Program#getDeclarationEmitPath

* Only iterate over enabled projects
@microsoft microsoft locked as resolved and limited conversation to collaborators Oct 21, 2025
This pull request was closed.
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.

2 participants