Skip to content
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

Dont include auto type reference directives on syntax only server #39472

Merged
merged 1 commit into from
Jul 7, 2020

Conversation

sheetalkamat
Copy link
Member

Detected while working on the goto def on syntax only server that we were including node_modules/@types auto type directives on syntax only server. This disables it.
I am not sure if this is desired behavior or not though

@DanielRosenwasser
Copy link
Member

Pretty sure I noticed this in the logs but wasn't sure if that's what the syntax server was always doing 😅. Good catch!

I think this is fine for our first iteration, but I could imagine expanding this to dependencies long-term like what @andrewbranch did for #37812. I think doing that, in conjunction with noResolve, would probably give people decent behavior.

@amcasey
Copy link
Member

amcasey commented Jul 7, 2020

As a general principle, I'm happy to strip out as much functionality as possible, because it's easy to add it back later. Having said that, I'm not sure I understand the consequences of this particular change. If we're going to make things like go-to-def and nav-to work across files (including closed files?), it seems like we'd want to include these files?

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Jul 7, 2020

I think the idea was that we can make go-to-definition work based on explicit import syntax rather than auto-including the world. Just as a reminder, the only thing we get from including all the @types packages is auto-import working better and making global declarations available (e.g. describe, require, and it) from @types/node, @types/mocha, etc.

@amcasey
Copy link
Member

amcasey commented Jul 7, 2020

@DanielRosenwasser I get that mixed up every time and it's only been a few weeks since @andrewbranch broke it down for me. 😅

@DanielRosenwasser
Copy link
Member

Well if any of this functionality was intuitive you wouldn't have heard so much about it 😄

@sheetalkamat sheetalkamat merged commit c08c059 into master Jul 7, 2020
@sheetalkamat sheetalkamat deleted the syntaxOnlyAutoTypeRefs branch July 7, 2020 20:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants