Enable more colors in typescript about typescript.el HOT 13 CLOSED

So far, as a fourth level of highlighting, I have implemented the following (see the first post for more detailed examples):

• function calls (font-lock-function-name-face)
• function definitions in classes (font-lock-function-name-face)
• decorators (font-lock-function-name-face)
• builtins (right now just console, as font-lock-type-face)
• access modifiers (private, public, etc. as typescript-access-modifier-face - inherits font-lock-keyword-face
• type hints in generics (font-lock-type-face)
• basic types (any, string, boolean, etc. as typescript-primitive-face - inherits font-lock-keyword-face
• moved constants (false, NaN, etc) to font-lock-variable-name-face for more contrast (only for this 4th level)
• this keyword (typescript-this-face - inherits font-lock-keyword-face)

The most important thing that is still left is type hints in general, some examples:

class Foo {
    private bar: Bar;
                 ^^^

    constructor(bar: Bar) {
                     ^^^
        this.bar = bar;
    }
}

The problem I have been having with highlighting these type hints is distinguishing them from simple object structures like { foo: bar } where bar in this case is actually a value, not a type. I'd certainly welcome any suggestions on that one - the only thing i have come up with is to assume user is going to give it a modifier like private/public, but since that's not rly needed it kind of sucks.

For what I have so far, I'm happy to open a PR and we can start to discuss some of the choices I've made etc.

from typescript.el.

FYI I have emailed Emacs-devel about additional font lock faces:

https://lists.gnu.org/archive/html/emacs-devel/2018-08/msg00020.html

from typescript.el.

All those changes sounds good to me, and if someone wants to put in the work to implement it, I will be happy to review and merge.

Right now we have 3 font-lock levels. If we're adding these new ones... Maybe that should go in a fourth?

If so, I'd support making the fourth one the new default, but I know there are people who in the past have thought that to be a bit "too busy", so they should have the option to keep things as they are (that is, customize to stay on level 3 or below).

Does that sound good?

from typescript.el.

Putting the additional highlighting in a 4th level seems reasonable. Though I wonder in practice whether it may make integration with other tools (minor modes, etc.) more complicated. I have no evidence one way or the other.

There's probably a segment of our user-base that would welcome the added fontification, but someone who wants to see this functionality integrated to typescript-mode needs to champion the change and implement it. In general, I'm in favor of adding fontification details, though I may not personally find them useful. But I have some reservations about some of the proposals made here.

I'm not in favor of marking window and document as builtins, because they do not always refer to the builtins. This is perfectly valid code:

function fnord(window: number): void {
   console.log(window); // `window` is not a builtin here
}

fnord(1);

(Case in point, Github's fontification erroneously marks window as a builtin, although it is not!!)

In workers neither window nor document are defined. And then there's code designed to run in Node.js, where they don't exist either. Having window and document fontified specially would be misleading in too many cases. The font-lock facilities are not powerful enough to accurately fontify them. And what about self in workers and global in Node.js? Fontifying builtins is a door I'd rather not open.

There's also the suggestion that Foo be given the face typescript-type-face in this fragment:

import { Foo } from 'bar'

Surely typescript-type-face cannot apply to all imported symbols, because some imported symbols are not types. You can import variables, functions, namespaces, etc. but typescript-mode cannot by itself determine which of the multiple possibilities applies. This brings back a comment I've made before in other issues filed here: Emacs's font-lock facilities are designed to support fontification logic that does not require a deep analysis of the code. They support looking at the immediate syntax of the code to make decisions. So for instance if you have const x = 1; const y = new x(), then the x in new x() will be fontified as a type name even tough x cannot be a type. The fontification code takes the presence of new at face value and infers from it that x is a type. In order for the fontification code to know that x is not a type, it would have to produce an AST (or the equivalent). The same problem occurs with imports: determining what kind of symbol is being imported requires an AST. This level of code analysis is the province of tide (by way of tsserver). As I've suggested in other fontification issues, it may be feasible to add to tide logic that augments the fontification done by typescript-mode. That logic would use tsserver to know what Foo actually is in import { Foo } from 'bar' and then apply the right face.

At first glance, the other cases appear unproblematic but I may have missed something.

from typescript.el.

@josteink & @lddubeau, I am happy to put some time into this. I'll just need some review comments when done, I assume particularly related to how best to integrate with what already exists.

To the point of customization, font-lock levels, and "too busy":

I would argue (only very slightly) against adding a new font-lock level and instead including this as level 3, for simplicity. I have to assume most people would welcome the added fontifications, as they do exist pretty ubiquitously in VsCode, Sublime, etc. Additionally, for people who like "less business", I can only assume that they are generally going to be using a fairly dulled out color theme anyway. Lastly, after pr #78 is complete, and assuming this is implemented in a similar fashion, the end user should always have full control of how things are displayed anyway, and as long as we document it clearly enough, I don't see why that should pose any problems.

@lddubeau to your reservations regarding the builtins and the imports, both very good points, and i agree, let's not include those.

@lddubeau to your last point regarding fontification based on actual code analysis: yes, that would be amazing if we could ultimately get that to work right, but I assume it is a much higher effort than a few regex's for highlighting which do the trick in 99% of cases. In fact, based on your example I tested how VSCode would highlight the following: const x = 1; const y = new x(); const z = x(); and it turns out even VSCode highlights those all differently.

from typescript.el.

I am happy to put some time into this

@tam5: That's great! Thanks!

Let us know when you have something you want reviewed, or if you want our feedback on how we think things may best be done. We'll be around!

to your reservations regarding the builtins and the imports, both very good points, and i agree, let's not include those.

I agree here. I guess that makes a full consensus.

to your last point regarding fontification based on actual code analysis: yes, that would be amazing if we could ultimately get that to work right, but I assume it is a much higher effort than a few regex's for highlighting which do the trick in 99% of cases

@lddubeau has some great points and thoughtful comments (as always).

This point about how highlighting based on types, etc can only be 100% accurate if powered if we have proper a code-analysis is as true as ever.

While I really respect his drive for accuracy in what this mode provides its users, I'm more lenient to pragmatic/practical which may not be entirely correct for all use-cases.

Basically doing proper code-analysis with regexps alone is really, really hard (if not impossible), and that we may get some false positives here and there should not stop us from implementing something which does the right thing most of the time.

Especially if the other option is no syntax-highlighting on something people commonly want syntax-highlighted.

So yeah. I think somewhere in the middle-ground we should be able to do some improvements which everyone agrees represents an improvement to the status-que...

(That ofcourse, while we all await that someone, somewhere writes this new and awesome tsserver based highlighting code :)

So... Go ahead and do your best. We'll be happy to review your patches!

from typescript.el.

any updates here?

from typescript.el.

@yintothayang I had started work on this at my fork, which I have now been using for a while, and honestly have not gotten around to rounding out the edges and making sure it is ready for a PR.

If you want to try it out, or check out the code and see what else needs to be done that would be great and we can potentially merge it in.

If you are writing Angular, you might also want to consider ng2-mode

from typescript.el.

I'll take a look, thanks!

from typescript.el.

Would love to see some progress on this! typescript-mode feels monotone in comparison to the competition :(

from typescript.el.

@tam5: Any change your fork is getting ready for a PR? :)

from typescript.el.

There's still some controversy/discussing about HOW such things should best be implemented, but I think having working code is a good start.

If you can create a PR, I won't guarantee that we will merge it right away, but I'm definitely eager to see what the code looks like, not to mention how it ends up looking to an end-user.

So yes, please create a PR and lets see where we can take this 😄

from typescript.el.

Closed and fixed by @tam5 :)

from typescript.el.