Skip to content

add return-to-declaration functionality on the tsserver branch #1192

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
merged 2 commits into from
Mar 19, 2017

Conversation

deanproxy
Copy link

[ ] All compiled assets are included (atom packages are git tags and hence the built files need to be a part of the source control)

@deanproxy deanproxy changed the title add return-to-declaration functionality add return-to-declaration functionality on the tsserver branch Feb 15, 2017
@@ -2,6 +2,15 @@ import {commands} from "./registry"
import {commandForTypeScript, getFilePathPosition} from "../utils"
import {simpleSelectionView} from "../views/simpleSelectionView"

const prevCursorPositions:any[] = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing the type annotation here. Once you add it, you'll discover a type mismatch between what getFilePathPosition returns and what open expects.

@@ -12,6 +21,8 @@ commands.set("typescript:go-to-declaration", deps => {
const client = await deps.getClient(location.file)
const result = await client.executeDefinition(location)

prevCursorPositions.push(location);
Copy link
Contributor

Choose a reason for hiding this comment

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

It's possible that the user executes this command, gets multiple results, but presses escape and doesn't open any of them. We don't want to record the previous position in that case - only if they make the jump.

open({
file: position.file,
start: { line: position.line, offset: position.offset }
});
Copy link
Contributor

Choose a reason for hiding this comment

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

The new object you're creating here has the exact same shape as position local, so there's no need for that. You could pass it directly, though you'll see that it's not what open expects.

Copy link
Author

Choose a reason for hiding this comment

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

Not completely sure what you mean here. position doesn't have a start attribute, it looks like:

{
    file: string,
    line: number,
    offset: number
}

Open expect the line and offset to be children of start, which is why I wrote it out like that. Are you saying to just change the definition of open to correspond with the position argument?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, gotcha, I didn't see the start bit. Carry on 😆

@deanproxy
Copy link
Author

added those suggested fixes. let me know if you see anything else.

@deanproxy
Copy link
Author

Just thinking about this... prevCursorPosition array can wind up growing too large and consuming memory if you don't return from your go-to's frequently and you're also one of those people like me that keeps their editor open all of the time. This isn't likely to be a big problem in most situations, but something to keep in mind.

@guncha guncha merged commit e9ba0f5 into TypeStrong:use-tsserver Mar 19, 2017
@guncha
Copy link
Contributor

guncha commented Mar 19, 2017

Thanks @deanproxy 👍

@TypeStrong TypeStrong locked and limited conversation to collaborators Jan 20, 2019
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