Skip to content

Conversation

@Phaiax
Copy link

@Phaiax Phaiax commented Jun 13, 2014

Hey

I've worked a little bit on the code in the dev branch.

Former i had the problem, that there was a bunch of unexecuted showError commands in the async tss queue if I type a character just every debounce_delay seconds. Every showError takes 5-10 seconds on my project so it could easily take up to a minute until i could see the most recent errors. This took CPU time and slowed the builds also.

I did a lot, these are the main changes:

  • still two instances of tss.js for each project (one for completions etc, one for the slow showErrors)
  • but both of them are async now
  • all commands are working with callbacks now, because there is no more synchron tss.js process
  • commands in the tss.js execution queue will be reordered, so that a showErrors will always work with the newest data
  • commands in the tss.js execution queue will be merged, so even if there are 3 showErrors in the queue, it will only be executed once
  • debouncing still enabled: showErrors will wait 0.8 seconds after the last keystroke
  • showErrors will only be executed if anything has changed on the open files.

This results in a much better responsiveness. Previously i had some delays (gui hang ups) on completion trigger etc, but now it works much smoother.

More changes:

  • refactoring, renaming, documentation of some files
  • Debug print statements in code

All these changes are working well with Linux and sublime 3.
When i'm done i can test it with sublime 2 (if they have a free version).
Can you test it in windows?

I'm open to any suggestions on my changes.

regargs, Dän

@Railk
Copy link
Owner

Railk commented Jun 13, 2014

Hello @Phaiax,

I've begin to look at your change yesterday and i'll try to test a look on windows and mac osx with your change this week-end.

You're change are really interesting :) and address problems i was trying to solve (i was considering going full python by translating typescript language service in python, but that's quite a task and a bit of maintenance problem after, to solve the laguiness)

if everything work well i'll merge asap. (i'll try to do that this week end)

thx for your contribution :)

@Phaiax
Copy link
Author

Phaiax commented Jun 13, 2014

In theory: Should the dev branch still work with sublime 2 or are all these ST3 statements just old crap?
Because plugin loading will fail directly in T3S.py on the first import statement for ST2 in line 16:

from lib.Commands import *
with
ImportError: No module named lib.Commands

@csnover
Copy link

csnover commented Jul 2, 2014

From an end-user perspective, this PR is great and seems to solve a severe performance problem in T3S that was forcing me to not use it. However, it needs at least the following two changes, otherwise it is very broken:

diff --git a/lib/Listener.py b/lib/Listener.py
index 51bc390..0fd15e6 100755
--- a/lib/Listener.py
+++ b/lib/Listener.py
@@ -137,7 +137,7 @@ class TypescriptEventListener(sublime_plugin.EventListener):
                COMPLETION.trigger(view, TSS)

                if not SETTINGS.get('error_on_save_only'):
-                       TSS.errors(view.file_name)
+                       TSS.errors(view.file_name())
                        #debounce(TSS.errors, self.error_delay, 'errors' + str(id(TSS)), view.file_name())


diff --git a/lib/Utils.py b/lib/Utils.py
index 5285e50..ce39e33 100755
--- a/lib/Utils.py
+++ b/lib/Utils.py
@@ -156,7 +156,7 @@ def read_file(filename):

 def read_and_decode_json_file(filename):
        """ returns None or json-decoded file contents as object,list,... """
-       f = read_file(f)
+       f = read_file(filename)
        return json.loads(f) if f is not None else None

 # FILE EXISTS

@Railk
Copy link
Owner

Railk commented Jul 2, 2014

i'll do the testing/merging next week as i'll be on holiday, sorry for the long delay.

@Phaiax
Copy link
Author

Phaiax commented Jul 3, 2014

There is another thing I want to implement, the auto completer has to check wheather it's the same dot it belongs to.

Type:
> this.          -> autocomplete triggers
>       foo.    -> autocomplete is ready and now displays the suggetions for this.

Thats sometimes confusing, but I will adress this issue during the next days.
And I will include csnovers changes.

Ah, and the errorview to source linkage is misbehaving.
If something is underlined red (error), but you resolve the issue in another file, the red underlining will remain until you make the next change to that first file.

Btw, I think its a good idea to indicate that there is a pending calculation of the current errors. Maybe I will use the first line of the error view to archive this.

@csnover
Copy link

csnover commented Jul 9, 2014

Here is another error that came up at some point which I assume is related to this PR:

File navigation : string indices must be integers
Traceback (most recent call last):
  File "T3S/lib/Commands.py", line 201, in async_react
    start_line = member['min']['line']
TypeError: string indices must be integers

This happened after doing an F5 project refresh.

Subsequent to that, when closing the project:

Traceback (most recent call last):
  File "T3S/lib/system/AsyncCommand.py", line 141, in <lambda>
    sublime.set_timeout(lambda:self.result_callback(tss_answer, **self.callback_kwargs),000)
  File "T3S/lib/Tss.py", line 223, in kill_and_remove
    PROCESSES.kill_and_remove(root)
  File "T3S/lib/system/Processes.py", line 84, in kill_and_remove
    self.get(root, SLOW).kill_tssjs_queue_and_adapter()
NameError: global name 'SLOW' is not defined

@Phaiax
Copy link
Author

Phaiax commented Jul 10, 2014

I will hopefully fix that this weekend

Phaiax added 2 commits July 11, 2014 18:16
eg Outline view has to be moved in main group
then restart of sublime
then close outline window
@Phaiax
Copy link
Author

Phaiax commented Jul 24, 2014

I only experienced those import errors in files which are not imported by any other file. Those files will still be added and updated to the typescriptservice and I think it causes undefined behaviour.

Is there any other case where those undefined imports can occour?

There is also this other issue which has been in T3S before: you can define multiple root files but only the first one will ever be loaded or used.

I think both problems are relatively easy to resolve, but may cause some bugs to appear which are harder to solve. Ill take a look.

On July 24, 2014 5:28:37 PM CEST, Colin Snover notifications@github.com wrote:

I don’t know if you plan on addressing it, but the one issue that still
exists for me (that existed and was worse before this patch) is where
the imported dependencies are not correctly loaded, resulting in files
that display false errors because tss thinks that an imported module is
a “non-module type”. Do you know what is going on there?


Reply to this email directly or view it on GitHub:
#63 (comment)

Diese Nachricht wurde von meinem Android-Mobiltelefon mit K-9 Mail gesendet.

@csnover
Copy link

csnover commented Jul 24, 2014

One way that seems to be possible to consistently reproduce the issue is:

  1. Ensure hot_exit is enabled in Sublime (it is by default)
  2. Open a TypeScript file that has an import statement
  3. Note that import loads OK and there are no errors
  4. Quit & reopen Sublime
  5. Note that import has not loaded and there are errors related to import
  6. Close & reopen tab
  7. Note that import loads OK and there are no errors

With regards to the root files…I honestly don’t understand why they are a thing that exists. When I open a TypeScript file, I want code intel for that file. The files that need to load are any open TypeScript file, plus the opened files’ explicitly defined dependencies (/// comments, import statements). I don’t understand what circumstances exist that one should have to specify a “root” file (badly written code without explicit dependencies?).

@Phaiax
Copy link
Author

Phaiax commented Jul 24, 2014

The root files are something that is related to typescript itself. If you type a compile command, you specify 'the root file'.

tsc "/home/foo/baar/main.ts" --module amd --target ES5 --outDir ...foobar...../build/js

Because typescript allows for circular imports, there is no logical way to determine the correct root file. Your only could get a clue to the directory, if you try to find the directory where the imports are relative to. But how far should you search? And what if you first open a library file located somewhere else.

So currently, if you open a typescript file and you did not specified or specified a non existent or wrong root file, it can't do anything. (The current problem is that he does something).

So for your example of reproducing: Do you have a root file specified and does that root file import the file you open? Do you have a .sublime-project file?

I will create some testprojects in the repository with all possible valid and invalid combinations just to be able to test.

@csnover
Copy link

csnover commented Jul 24, 2014

Because typescript allows for circular imports, there is no logical way to determine the correct root file.

I believe the “root” file would be the file that is open in the currently focused tab.

Circular dependencies between types are fine in TypeScript, even between those that are imported. AFAIK the only time there is a circular dependency problem is when you load external modules at runtime, where the order in which they are loaded determines which module is treated as the circular dependency (a -> b -> a or b -> a -> b). This is not a problem in the compiler, but a limitation of the way that JavaScript works. It should not impact the static analysis of TypeScript.

Your only could get a clue to the directory, if you try to find the directory where the imports are relative to. But how far should you search? And what if you first open a library file located somewhere else.

The way the TypeScript compiler resolves dependencies is very straightforward, and if it doesn’t already, T3S should take the same approach to ensure correct interoperability.

For absolute module IDs, the compiler looks for <module id> + '.ts', then <module id> + '.d.ts', up the directory chain starting from the directory of the dependent file until it reaches the root of the filesystem.

For relative module IDs, you just resolve the relative ID relative to the dirname of the dependent file, <dirname> + <module id> + '.ts', then <dirname> + <module id> + '.d.ts'.

An approach where the open files in the editor are the ones that are used as dependencies for other open files in the editor leads to an incredibly brittle and badly functioning design, one that does not match how the compiler works and one that leads to false positives & false negatives in the editor.

So currently, if you open a typescript file and you did not specified or specified a non existent or wrong root file, it can't do anything. (The current problem is that he does something).

I don’t know what this means; I have to do this all the time (using a bogus root file) and it pretty much works. I am a library developer; there is no “root file” for a library.

So for your example of reproducing: Do you have a root file specified and does that root file import the file you open? Do you have a .sublime-project file?

I have a .sublimets with a bogus root file.

@Phaiax
Copy link
Author

Phaiax commented Jul 25, 2014

Ok, I've thought about it and tested what the typescript-services are capable of:

At first It's no general problem to use the first open .ts file to init the typescript-services. The only drawback is the following situation: You have a (big) project and most of the files are closed.

lets say the imports are as following: main.ts -> lib.ts -> utils.ts

  1. You open lib.ts.
  2. The typescript-services init with this file. (utils.ts will be added to the internal model of the applcation; main.ts will not be recognized by the typescript-services)
  3. You change a function name in lib.ts. No error will be displayed for any now invalid function call in main.ts

The error would be recognized if the typescript-services were always initialized with the file from which all imports are starting.

And I was able to reconstruct your error from above:

Files: main.ts (root) >imports> lib.ts
anotherfile.ts >imports> utils.ts
Open editor with the file anotherfile.ts first.
The import of utils.ts will fail. That's a 'bug' in the typescript-services. In theory it would be no problem to just search for that imported file and add it. But it doesn't do so, so we have to deal with it.

It only automatically adds files which have an import-line-of-sight to the "root"-file the typescript-services were inited with.

There are two ways to resolve this:

  • First like you did with reopening: Just open utils.ts in another tab. T3S currently adds every .ts file to the typescript-services, so this will cause the error to disappear.
  • Creating an import-of-sight to the rootfile: maybe by importing anotherfile.ts in lib.ts. That error will still remain, even if you trigger error-compilation again by saving or adding a space in a random .ts file. Only if you hit F5 afterwards (which triggers the 'reload' command of the typescript-services), the typescript-services will probably recreate their import tree and the error will disappear.

So i draw the following conclusions. Correct me if you think i'm wrong.

  • Rootfiles are necessary to import every file at init time to be able to make error detection in all files (first example)
  • There is no need to force people to use root files. If they have a stand-alone (or two or three..) .ts files they probably only want some error correction for that file.
  • Even for those people it's very simple to just create a .sublimets file with the wizard.

so

  • I would still force people to create a typescript project or a .sublimets file.
  • I would update the documentation about rootfiles to something like this:
    • Rootfiles: You must set a rootfile. Use that file as rootfile which directly or indirectly imports any other typescript file in your project. You can specify multiple rootfiles if you have multiple roots in your import tree.
  • I would display additional hints in the errorview if import error statements are detected:
    • // Make sure you have a straight chain of imports from any of your rootfile to {the file with the error}. Then press F5 (aka Typescript: Reload command in command palette)
    • // If you don't want that straight chain of imports, just add another rootfile to your sublime-project. You maybe need to convert your .sublimets file to a <>.sublime-project file. Refer to the T3S Readme.

I think that would be ok for everyone, even you as a library developer.

Btw: you can take a look at the watched files aka the files in the import tree of typescript-services.
Type this into the python console:
import T3S
T3S.lib.system.Files.FILES.update_indexed_files(sublime.active_window().active_view().file_name())
print("\n".join(T3S.lib.system.Liste.LISTE.liste.keys()))

I also fixed a bug which caused an error in when T3S tried to refer to the settings for a non-indexed .ts file. I will push soon.

@Phaiax
Copy link
Author

Phaiax commented Jul 29, 2014

@Railk I would be pleased if you could comment my last comment.
And maybe you can explain what rootfiles are for in your opinion, or explain me the concept in a few words.

@Railk
Copy link
Owner

Railk commented Jul 30, 2014

Sorry life happened, my free time went away and i've been busy since.
I'll comment your last comment as soon as possible, i prefer not to give time schedule right now but it's high on my todo list to check everything, i already began the merge and test on my local machine but could not finish yet...

@csnover
Copy link

csnover commented Jul 30, 2014

@Phaiax Thanks for your thoughtful response. I’m planning to read through it in greater detail and respond to any points when I have a free moment.

@pocesar
Copy link

pocesar commented Sep 30, 2014

@Phaiax can you merge the #64 PR and release the package on Package Control under a new name?

@Phaiax
Copy link
Author

Phaiax commented Sep 30, 2014

I can do this, but i'm in the usa currently without my laptop, so you'll have to wait until november.

Diese Nachricht wurde von meinem Android-Mobiltelefon mit K-9 Mail gesendet.

@pocesar
Copy link

pocesar commented Sep 30, 2014

@Phaiax thanks, that would be great 👍

@alexgorbatchev
Copy link

ping

@Phaiax
Copy link
Author

Phaiax commented Nov 17, 2014

I'm trying it in the next few days, but no promise :D

@KATT
Copy link

KATT commented Dec 3, 2014

pong

@Phaiax
Copy link
Author

Phaiax commented Dec 3, 2014

Jep i know.. I'm participating in an Hackathon on Friday so its done this weekend probably

Diese Nachricht wurde von meinem Android-Mobiltelefon mit K-9 Mail gesendet.

@Phaiax
Copy link
Author

Phaiax commented Jan 5, 2015

Hey

I've created a new plugin, which is based on T3S and includes these patches.

https://github.com/Phaiax/ArcticTypescript

You can install ArcticTypescript using PackageControl. Please remove T3S first, both plugins together are incompatible.

  • untested on windows and mac, but i will fix bugs as soon as you create Issues for ArcticTypescript.
  • major updates which will break configuration structure may happen in future

Phaiax

@druppy
Copy link

druppy commented Jan 5, 2015

@Phaiax thanks, this seems like a god plan, I like your #63 a lot so this seems like more of that ! ArcticTypescript ... here I come :-)

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.

7 participants