Skip to content

Fix bug: Get mapped location of definition for findAllReferencesFull #27113

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

Merged
1 commit merged into from
Sep 17, 2018

Conversation

ghost
Copy link

@ghost ghost commented Sep 15, 2018

Fixes #26175

@ghost ghost requested review from sheetalkamat and amcasey September 17, 2018 17:57
@amcasey
Copy link
Member

amcasey commented Sep 17, 2018

I think @uniqueiniquity was having some trouble in related scenarios.

@uniqueiniquity
Copy link
Contributor

@amcasey I did, but I'm not sure whether I had my project configured correctly or not.

function combineProjectOutputWorker<TLocation extends sourcemaps.SourceMappableLocation | undefined>(
projects: Projects,
defaultProject: Project,
initialLocation: TLocation,
projectService: ProjectService,
cb: (where: ProjectAndLocation<TLocation>, getMappedLocation: (project: Project, location: sourcemaps.SourceMappableLocation) => boolean) => void,
cb: CombineProjectOutputCallback<TLocation>,
Copy link
Member

Choose a reason for hiding this comment

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

Personally, I find it confusing that some callers pass a callback with a different goal (tryAddToTodo) and no intention of returning. Maybe the return type should also be a type parameter?

Copy link
Author

Choose a reason for hiding this comment

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

You mean no intention of using the return value of getMappedLocation? Though every caller uses the return type of at least to test whether there was something added. It's true that they use it like a boolean but I don't think there's a type-safe way to have the return value be a type parameter that's either boolean or sourcemaps.SourceMappableLocation | undefined.

@ghost ghost merged commit bfc0093 into master Sep 17, 2018
@ghost ghost deleted the findAllRefsFull branch September 17, 2018 21:14
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants