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

Syntax only server creates inferred project with all the open files w… #38561

Merged
merged 8 commits into from
Jun 16, 2020

Conversation

sheetalkamat
Copy link
Member

…ith noResolve and can handle semantic operations

Fixes #

…ith noResolve and can handle semantic operations
@sheetalkamat
Copy link
Member Author

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented May 13, 2020

Heya @sheetalkamat, I've started to run the tarball bundle task on this PR at 5726838. You can monitor the build here.

@sheetalkamat
Copy link
Member Author

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented May 13, 2020

Heya @sheetalkamat, I've started to run the tarball bundle task on this PR at 90d8a96. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented May 14, 2020

Hey @sheetalkamat, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/73860/artifacts?artifactName=tgz&fileId=2BC1D5FE49C2AD5CCA135D57CEE4CA308A5E08ADDD7B5A264EDC56EE6D487BE002&fileName=/typescript-4.0.0-insiders.20200514.tgz"
    }
}

and then running npm install.


There is also a playground for this build.

@sheetalkamat
Copy link
Member Author

@mjbvz @minestarks @uniqueiniquity @amcasey @DanielRosenwasser @RyanCavanaugh
#38561 (comment) is build where syntaxOnly server can accept semantic commands.. ( i havent added any custom changes for any semantic commands nor have i limited any semantic operations as part of this prototype)

#38564 is PR that is on top of this and makes normal tsserver behave like syntaxOnly (one with semantic operations allowed)

@amcasey
Copy link
Member

amcasey commented May 14, 2020

I'll be interested to see what this does to startup perf on the syntax server. Looks pretty cool though.

@amcasey
Copy link
Member

amcasey commented May 22, 2020

I've run into a pretty big issue on the VS side: semantic operations tend to be triggered for Roslyn Documents and those don't exist until the project is loaded. So I see operations during project load in already open documents, but not in the newly opened document (i.e. the one that triggered the project load). Obviously, this has nothing to do with the tsserver change.

@DanielRosenwasser
Copy link
Member

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 9, 2020

Heya @DanielRosenwasser, I've started to run the tarball bundle task on this PR at 90d8a96. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 9, 2020

Hey @DanielRosenwasser, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/75966/artifacts?artifactName=tgz&fileId=BB849547A1DF11A54E5245D89DF5DAFE50EAC3E3C08A813981103A387395A0B802&fileName=/typescript-4.0.0-insiders.20200609.tgz"
    }
}

and then running npm install.


There is also a playground for this build.

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Jun 9, 2020

I'm not able to see things working better with

@DanielRosenwasser
Copy link
Member

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 11, 2020

Heya @DanielRosenwasser, I've started to run the tarball bundle task on this PR at 90d8a96. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 11, 2020

Hey @DanielRosenwasser, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/76193/artifacts?artifactName=tgz&fileId=0AD85EA4EF3D7253C964378CA736CCDB891DD80E981648EE08731F97511241AD02&fileName=/typescript-4.0.0-insiders.20200611.tgz"
    }
}

and then running npm install.


There is also a playground for this build.

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Jun 11, 2020

I'm finally trying this out with VS Code Insiders! Pretty cool! A couple of thoughts and ideas

From the editor side:

  • the startup time still isn't instantaneous for the syntax server, so I'm really never sure whether I can start editing reliably until I try quick info over and over. I think this is something we'll really need to provide a visual indication for before we make it broadly available.

From the TypeScript side:

  • I think cross-file go-to-definition was one of the key scenarios we'd like to get to users, so I think we should really try to get that to light up, either here or in a follow-up PR.
  • the fact that we show any when things aren't resolved in this mode is kind of confusing. For type display purposes, we might want to consider displaying the error type as something like a loading type.

@CyrusNajmabadi, how does Roslyn work when types haven't been resolved yet in things like quick info and signature help?

For example we have this signature

image

but CodeActionsState.State can't be resolved, so signature help looks like this:

Untitled

@CyrusNajmabadi
Copy link
Contributor

@CyrusNajmabadi, how does Roslyn work when types haven't been resolved yet in things like quick info and signature help?

We don't have that concept :)

@sheetalkamat sheetalkamat marked this pull request as ready for review June 13, 2020 00:06
CommandNames.CompileOnSaveEmitFile,
CommandNames.CompilerOptionsDiagnosticsFull,
CommandNames.EncodedSemanticClassificationsFull,
CommandNames.SemanticDiagnosticsSync,
Copy link
Member Author

Choose a reason for hiding this comment

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

SyntacticDiagnosticsSync: This is somewhat project setting dependent ? Atleast at some point parsing errors use to be different based on target .. eg target determines what unicode is considered identifier start... So i am not sure if this should be enabled.. But if we do enable we i was wondering if GetErr should only do syntax checks and skip semantic and suggetion diagnostics on syntax server

Some questionable which i have disabled for now
Reload
ReloadProjects
PrepareCallHierarchy
ProvideCallHierarchyIncomingCalls
ProvideCallHierarchyOutgoingCalls

@sheetalkamat
Copy link
Member Author

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 13, 2020

Heya @sheetalkamat, I've started to run the tarball bundle task on this PR at 89127a5. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 13, 2020

Hey @sheetalkamat, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/76345/artifacts?artifactName=tgz&fileId=CD3C20B0D73291C13F028F1A072CA740FDD866AD700C026ACA443AFA3B0B0C3002&fileName=/typescript-4.0.0-insiders.20200613.tgz"
    }
}

and then running npm install.


There is also a playground for this build.

Copy link
Member

@amcasey amcasey left a comment

Choose a reason for hiding this comment

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

Some comments, but LGTM

Gulpfile.js Outdated Show resolved Hide resolved
src/server/session.ts Outdated Show resolved Hide resolved
@@ -1171,6 +1171,26 @@ namespace ts {
}
}

const invalidOperationsOnSyntaxOnly: readonly (keyof LanguageService)[] = [
Copy link
Member

Choose a reason for hiding this comment

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

How does this relate to the list in session.ts? Do they need to stay in sync? Is one redundant?

if (syntaxOnly) {
invalidOperationsOnSyntaxOnly.forEach(key =>
ls[key] = (...args: any[]) => {
throw new Error(`LanguageService Operation: ${key} not allowed on syntaxServer:: arguments::${JSON.stringify(args)}`);
Copy link
Member

Choose a reason for hiding this comment

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

As above, we probably don't need the arguments.

@DanielRosenwasser
Copy link
Member

Speaking of logs- do editors log from the syntax server?

@amcasey
Copy link
Member

amcasey commented Jun 16, 2020

VS logging is opt-in (via env var or regkey), but, yes, it applies to both.

VS Code only has a command to show the semantic log (AFAIK), but I'm pretty sure it enables both.

@sheetalkamat
Copy link
Member Author

@mjbvz We definitely want users to be able to share the syntaxOnly server logs easily.

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