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

tsserver feature request: improve compiling functionality #19492

Closed
anstarovoyt opened this issue Oct 26, 2017 · 23 comments
Closed

tsserver feature request: improve compiling functionality #19492

anstarovoyt opened this issue Oct 26, 2017 · 23 comments
Labels
Duplicate An existing issue was already created

Comments

@anstarovoyt
Copy link

First of all thanks for implementing the feature: #19336
It allows us to remove own implementation of the command "reload" in the next versions of WebStorm.

In the latest WebStorm release we have rewritten the service integration and now we use own extension of tssever only for compiling.

For complete removal the extension we need a couple new features:

  1. Add a new API method for compiling projects by tsconfig.json.
  • It must return a list of diagnostics, emitted and affected files.
  • Optional flag to skip check 'compileOnSave' value;
  • Optional flag "forced" (similar to CompileOnSaveEmitFile)
  1. Add new optional flags to CompileOnSaveEmitFile:
  • option to skip checking 'compileOnSave' value;
  • option to return list of diagnostics, emitted and affected files;
  • option projectFileName;
@DanielRosenwasser DanielRosenwasser added API Relates to the public API for TypeScript Suggestion An idea for TypeScript labels Oct 26, 2017
@mhegazy
Copy link
Contributor

mhegazy commented Oct 26, 2017

If i understand the requests correctly, these are all achievable in the API today. The API is intentionally split into smaller pieces to avoid long running operations on the server. the server is single threaded, and has no way to correctly interrupt/resume long running background tasks. we defer to the host to schedule these and cancel them as needed.

@mhegazy mhegazy added the Needs More Info The issue still hasn't been fully clarified label Oct 26, 2017
@anstarovoyt
Copy link
Author

anstarovoyt commented Oct 26, 2017

@mhegazy

If i understand the requests correctly, these are all achievable in the API today

no, we cannot achieve some of the functions using current API (without a session extension).

Right now I see these problems:

  1. CompileOnSaveEmitFile doesn't return list of output files (I mean ".js" and ".js.map" files). We need this information for refreshing the files in the IDE. There is no way to get the output files (please correct me if I am not right)
  2. getCompileOnSaveAffectedFileList doesn't return files if compileOnSave = false.

@mhegazy
Copy link
Contributor

mhegazy commented Oct 26, 2017

CompileOnSaveEmitFile doesn't return list of output files (I mean ".js" and ".js.map" files). We need this information for refreshing the files in the IDE. There is no way to get the output files (please correct me if I am not right)

the file would change on disk, is not that sufficient to update your state? we can make it send a response with the list of files written. but seems weird that you need that to refresh your state, these files could change in an out-of-band fashion any ways, e.g. a command line build command outside the editor.

getCompileOnSaveAffectedFileList doesn't return files if compileOnSave = false.

not sure what you expect here. if the user chose to disable the feature why do you want to force enable it?
why is this different from any other flag, e.g. typeAquision, or noEmitOnError?

@anstarovoyt
Copy link
Author

anstarovoyt commented Oct 26, 2017

but seems weird that you need that to refresh your state, these files could change in an out-of-band fashion any ways, e.g. a command line build command outside the editor.

We use this information in several ways:

  1. Exclude files from the project (we don't want to index generated files)
  2. If file was created first time (and for example parent directories) it can take a long time to get the information from FS but we get it immediately using the compile result.

not sure what you expect here. if the user chose to disable the feature why do you want to force enable it?

We use "compileOnSave" flag for compiling on the fly. But WebStorm also has the explicit button "compile"(and "before run" task for compiling ts files) and users expect that the compiling will not taking into account the "compileOnSave" option. In fact it is not "compileOnSave" it just "compile" using the service.

@mhegazy
Copy link
Contributor

mhegazy commented Oct 26, 2017

We use this information in several ways:

Should be a simple change to add. we can return a list of files written to disk.
Feel free to send us a PR for the change.

We use "compileOnSave" flag for compiling on the fly. But WebStorm also has the explicit button "compile"(and "before run" task for compiling ts files) and users expect that the compiling will not taking into account the "compileOnSave" option. In fact it is not "compileOnSave" it just "compile" using the service.

Seems like you have a custom setup there. we make some assumptions based on compile-on-save being set, and decide not to keep a builder for instance. it should be possible for you to override the implementation of CompileOnSaveAffectedFileList in session, the functions it call are all public. I do not think this should be a supported scenario in the server through a new command.

@mhegazy mhegazy added Help Wanted You can do this and removed Needs More Info The issue still hasn't been fully clarified labels Oct 26, 2017
@anstarovoyt
Copy link
Author

anstarovoyt commented Oct 26, 2017

it should be possible for you to override the implementation of CompileOnSaveAffectedFileList in session, the functions it call are all public. I do not think this should be a supported scenario in the server through a new command.

Yes, we have a similar solution for the problem. But the session api (even public) is not reliable (in
2.6RC you broke ~ 5 public methods that we used in previous versions).

Anyway thank you, I will try to send a PR for the first scenario.

@mhegazy
Copy link
Contributor

mhegazy commented Oct 26, 2017

Yes, we have a similar solution for the problem. But the session api (even public) is not reliable (in
2.6RC you broke ~ 5 public methods that we used in previous versions).

Sorry about that. we have added new tests to check the API changes at build time. so hopefully we do not get these unintentional changes in future versions.

@angelozerr
Copy link

Thanks @anstarovoyt for creating this issue. I had created a similar issue than you at #17630

@anstarovoyt I tell me how to you support the case of user save a ts file which must be excluded. I suggested

compileOnSaveAffectedFileList should throw error when the given file is excluded of tsconfig.json. It will give the capability to display an error dialog when user wishes to compile a ts file which is excluded by tsconfig.json

and @mhegazy answered me:

do not you have this already from projectInfo?

Thanks for your answer.

@anstarovoyt
Copy link
Author

anstarovoyt commented Oct 26, 2017

@angelozerr as I said before we extend the ts session and have own commands so we can do all the required checks on the tsserver side

@mhegazy
Copy link
Contributor

mhegazy commented Oct 27, 2017

I forgot about #17630. this issue seems like a duplicate of #17630 then. thanks @angelozerr .

closing in favor of #17630.

@mhegazy mhegazy added Duplicate An existing issue was already created and removed API Relates to the public API for TypeScript Help Wanted You can do this Suggestion An idea for TypeScript labels Oct 27, 2017
@angelozerr
Copy link

@angelozerr as I said before we extend the ts session and have own commands so we can do all the required checks on the tsserver side

OK I understand more now, but your goal is not to use tsserver instead of implementing your own ts session? Perhaps tsserver should provide a new isInScope(fileOrDirName) command?

@mhegazy
Copy link
Contributor

mhegazy commented Oct 27, 2017

you should be able to get that with ProjectInfo command.

@angelozerr
Copy link

Sorry @mhegazy you have already suggested me this solution and I hav enot give you an answer. Indeed I could consume ProjecInfo command but I'm afraid with performances.

In Eclipse builder word, we receive a list of files or folders to compile, and you need to filter it ot not. If tsserver could provide an isInScope(fileOrDirName) command, I could speed the build to avoid calling CompileOnSaveAffectedFileList for each ts files which must be excluded (ex : inside node_modules)

@mhegazy
Copy link
Contributor

mhegazy commented Oct 30, 2017

maybe i do not understand what isInScope supposed to do.. can you elaborate.

@angelozerr
Copy link

Ok my goal is to use tsserver to compile ts files instead of using tsc with watch mode:

  • I can manage progress monitor and stop compilation process when user click on stop.
  • it avoid having double parsing (one for tsserver and one for tsc)
  • I can consume language services (like tslint), but perhaps tsc can support that?

When an Eclipse project is opened, an Eclipse builder is executed and loop for each files of the project. In each loop I check if ts file must be compiled by calling CompileOnSaveAffectedFileList. Imagine you have a lot of ts files in your node_modules folder, I will check for each ts files of node_modules if ts y calling file must be compiled by calling CompileOnSaveAffectedFileList, although if I could consume isInScope("node_modules") which returns false in this case, I could stop the loop.

You suggested me to consume ProjectInfo, but this command works only if you know a ts file which belong to a ts project. Me I don't know ts file which belong to a ts project. I could have several tsconfig.json files. If projectInfo could work by calling it with tsconfig.json file (instead of ts file), I could perhaps consume it:

  • I could track the list of tsconfig.json in the project with Eclipse API.
  • call for each tsconfig.json the projectInfo to build a list of ts files to compile. I could use this list to check if ts file must be compiled or not (like you suggested me).

But I'm a little afraid with performance when projectInfo is called with a lot of ts files (the JSON response could be big).

To support compile with tsserver with clean mean and good perforamnce, I will need:

  • call projectInfo with tsconfig.json file to support full build. Perhaps it will support the feature of @anstarovoyt (But WebStorm also has the explicit button "compile")
  • provide an isInScope(fileOrDirName) which basicly will call projectInfo and check if it contains fileOrDirName. isInScope returns just a boolean compare to projectInfo which returns a big ts files list.

Hope you will understand my need.

@mhegazy
Copy link
Contributor

mhegazy commented Oct 31, 2017

Imagine you have a lot of ts files in your node_modules folder, I will check for each ts files of node_modules if ts y calling file must be compiled by calling CompileOnSaveAffectedFileList, although if I could consume isInScope("node_modules") which returns false in this case, I could stop the loop.

That is the part i am confused about. you have some starting point, like a list of open files for instance, get the projectInfo for these, and that all you need to build.

If projectInfo could work by calling it with tsconfig.json file (instead of ts file), I could perhaps consume it:

We could make that work. but earlier you said the build loop works on .ts files and not the tsconfig.json.

@angelozerr
Copy link

That is the part i am confused about. you have some starting point, like a list of open files for instance,

The list of files is not necessary opened. When you open an Eclipse project the first time, ther eare none opened files, but builder is called with a list of files (closed) of the project.

get the projectInfo for these, and that all you need to build.

You suggested me to consume projectInfo for each files of the list? If it that:

  • I'm afraid with performance.
  • if file is not opened, I think projectInfo doesn't work.

We could make that work. but earlier you said the build loop works on .ts files and not the tsconfig.json.

the build loops for the whole files of the project (*.java, *.ts, *.json files ), but in my case I filter only ts files.

@mhegazy
Copy link
Contributor

mhegazy commented Oct 31, 2017

do you assume all projects have tsconfig.json?

@angelozerr
Copy link

do you assume all projects have tsconfig.json?

Yes, I assume even that a projct can contains several tsconfig.json files.

@mhegazy
Copy link
Contributor

mhegazy commented Oct 31, 2017

so filtering the tsconfig.json files, and then calling projectInfo would be a workable solution for you? if so, as i noted earlier we can make projectInfo work on tsconfig.json as well as .ts/.js files.

@angelozerr
Copy link

so filtering the tsconfig.json files, and then calling projectInfo would be a workable solution for you?

I think it can be a good solution.

if so, as i noted earlier we can make projectInfo work on tsconfig.json as well as .ts/.js files.

It should be very cool!

@mhegazy
Copy link
Contributor

mhegazy commented Oct 31, 2017

A PR would be appreciated.

@typescript-bot
Copy link
Collaborator

Automatically closing this issue for housekeeping purposes. The issue labels indicate that it is unactionable at the moment or has already been addressed.

@microsoft microsoft locked and limited conversation to collaborators Jun 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Duplicate An existing issue was already created
Projects
None yet
Development

No branches or pull requests

5 participants