Makes typescript a derived mode of js-mode #45
Conversation
This commit removes most of the code copy pasted from js-mode. The TypeScript specific behavior is implemented by the methods typescript-indent-line and typescript--font-lock-keywords.
|
This PR is primarily to get feedback on the approach I have taken with making latest js-mode changes available to typescript-mode. The indentation test is failing. See the gist here with the differences https://gist.github.com/sudarshang/d7e8a86de1cb95b97738ebfdaa4aaee4 Todo items:
Please let me know if the approach taken here looks OK to you? I would greatly appreciate if anyone can check this branch out and use this for a while to see if there are any benefits to going this route. |
|
Thanks for putting time and effort into this! I'm on vacation this week with no computer, but will take a serious look at this some time next week. |
| :group 'typescript) | ||
| (cons js--constant-re font-lock-constant-face))) | ||
| "Level two font lock keywords for `js-mode'.") | ||
|
|
There was a problem hiding this comment.
Shouldnt the doc-string here say typescript-mode?
| (typescript--inside-pitem-p pitem)) | ||
| do (pop pstate)) | ||
| "Level three font lock for `js-mode'.") | ||
|
|
There was a problem hiding this comment.
Same about this reference to js-mode. Shouldn't it be typescript-mode?
| (list typescript--function-heading-1-re 1 font-lock-function-name-face) | ||
| (list typescript--function-heading-2-re 1 font-lock-function-name-face)) | ||
| "Level one font lock keywords for `typescript-mode'.") | ||
|
|
There was a problem hiding this comment.
Shouldn't we instead make this an alias for js--font-lock-keywords-1 and reference this in higher levels for improved consistency? Outright removing it seems a bit much IMO.
| (list (cons 'c typescript-comment-lineup-func)))) | ||
| (c-get-syntactic-indentation (list (cons symbol anchor))))) | ||
| "Font lock keywords for `js-mode'. See `font-lock-keywords'.") | ||
|
|
There was a problem hiding this comment.
Same naming-nitpick about js-mode here.
| (setq-local open-paren-in-column-0-is-defun-start nil) | ||
| (setq-local font-lock-defaults (list typescript--font-lock-keywords)) | ||
| (setq-local syntax-propertize-function #'js-syntax-propertize) | ||
| (setq-local prettify-symbols-alist js--prettify-symbols-alist) |
There was a problem hiding this comment.
Do we need to explicitly re-set the js-* functions in our derived mode? Shouldn't we inherit these from js-mode?
| (set (make-local-variable 'parse-sexp-lookup-properties) t) | ||
| (setq-local parse-sexp-ignore-comments t) | ||
| (setq-local parse-sexp-lookup-properties t) | ||
|
|
There was a problem hiding this comment.
Same for other local variables like these. These are already set in js-mode, so do we really need to be explicit about them here?
What if js-mode is updated to reflect new syntax... If we keep these doubled here, won't they cause a needless maintenance burden for us?
|
I'm back from my vacation, so now I've finally been able to look at these changes in more detail. Most changes seems to be outright removing code and changing references to existing code we'd like to not have to reimplement. The amount of monkey-patching and glue on top of In short: I think you've done a phenomenal job here!
It seems most of the indentation errors are caused by standardized ES-features which aren't yet properly supported by Basically... Do we risk that this becomes a liability in the future? And how easy will it be to monkey-patch these stuff on top of
I did some very preliminary testing... and from what I can tell CC: @lddubeau, @ananthakumaran : Do you guys have any opinions on this effort? I'd appreciate if more people than just me chip in :) |
|
good ~~ |
|
@josteink AFAICT,
Can you double-check your results? Because I sure don't get the same indentation here when running |
| "each")) | ||
| "Regexp matching keywords optionally followed by an opening brace.") | ||
|
|
||
| (defconst typescript--indent-keyword-re |
There was a problem hiding this comment.
I suspect the removal of the code that used this constant explains the failing tests with in and instanceof. Unsure what the answer is. Either the logic that was using these symbols (typescript--looking-at-operator-p and typescript--continued-expression-p) has to be reinstated, or the derivation from js-mode has to incorporate somehow the logic that these functions were performing.
|
@josteink Thanks for your review. I am traveling right now and can start addressing the issues you raised next week. |
@lddubeau You're right. My bad. I was looking at pre-saved @sudarshang: Please disregard my second comment about indentation. In fact I'll delete it to avoid any future confusion based upon it. |
|
@lddubeau wrote:
I guess this is what I meant by this comment earlier on:
If we end up with something which has less code, but is more work to maintain, that's not really a net win either. As someone who has recently dabbled a bit with the formatting-logic, do you have any opinion or gut feeling about this change? |
|
@josteink When it comes to indentation I'd expect it is better to roll our own. A lot of the problems I fixed with indentation were due to elisp code that treated TS code as if it were JS code. It is possible to monkey-patch but there's a line beyond which monkey-patching can become counter-productive as it makes the logic hard to follow. |
|
Do I read that as you would prefer for most of the indentation-related code to stay intact instead of using the Even with those bits remaining, I still think we'd be left with a net gain. Just look at the savings in the font-locking section. |
|
@josteink Yeah, I was being telegraphic there. Thinking about this more, I'm going to put it this way. It may be better to think through how the indentation code could be modified to remove duplication with On a different note. One thing just occurred to me now... and correct me if I missed something but this change would introduce breakage across the board for users who have customized There's a similar problem with functions that are not marked internal to the mode, and are thus "fair game". If I replaced In the past, I have actually also replaced functions that are marked internal, like the indentation functions, with my own customizations. (These customizations became the indentation fixes PRs that I submitted here.) I know we're not supposed to override internals, but still, I wonder how many users of the mode do it anyway and would be affected. We're at risk of having users complain about the names they used in their customizations disappearing or changing. |
As such, leaving all the current code there for now, and looking into how we can slim it down later might be the best approach?
This again backs up your first point.
Yes. That's a correct observation. The more we inherit directly from I honestly think that makes sense, as I want those 2 development-environments to be as similar as possible, but like you say, it's a breaking change in the sense that:
The best would ofcourse be if we somehow can find a way around that. Do you have any idea if we could create buffer-local variables for the I mean... It's hacky to a certain degree, but it's contained hackyness and not hackyness interspersed all over the general code-base. So if something like that works, I could be OK with that. Either way, if we go through with this, I would propose that we bump the major version number to signal that this is a major release (and if there's no way around it, that there will be some breaking changes). |
Yes.
I cannot think of a way that would preserve the current behavior. Taking the method you describe literally, one issue would be that modifying the customized variables would not immediately affect |
Then it's agreed. That's how we do it. @sudarshang: Could you revert these sections?
Good point. That's the best solution I can currently think of though. Do you have any other ideas or options you think we could consider instead? |
|
Have other typescript devs have had a chance to use this branch? I was teaching myself TypeScript and ended up taking a detour to teach myself elisp and improve the typescript-mode. (How about that Yak Shaving?). I have only used this branch for a couple of days. I never used the original typescript mode so other than test failures I am not aware of ways in which this branch is inconsistent with the original mode. I am OK with anyone else taking over this branch and fixing the issues that have been raised. It should be easier for someone who has more experience with the original code to figure out how to keep the original code along with the new js-mode code. |
I can't say for sure, but I do that suspect only the people chipping in here are paying attention.
I ended up learning elisp the same way, although with Common LISP as the actual thing I was trying to learn :)
I think we have some pretty extensive test-cases. I don't for a second think they are flawless, but if they pass, I think they're thorough enough that I'm confident enough to release a new version.
At the office I'm pretty occupied with non-TS work for the moment, and expect to be so for at least some time into the future. I think the changes we've suggested should be fairly doable, even if you lack elisp-experience. And honestly... You've already managed to do a whole lot of good work here. Much more than I would be able to on my initial elisp-projects ;) I say you started it, and you should get the honour of landing it. Go for it! And really: There's no rush here. Take whatever time you feel you need. |
To confirm, you want to keep all the indentation code as it is? So |
|
That's correct. |
|
@josteink @lddubeau I added back the indentation code. However, I see many more test case failures now. See here https://gist.github.com/sudarshang/b95907de5390f07be29e6d896af315ed Do let me know if there is something obviously wrong with what I am doing. |
|
Thanks for the update! Looks to me like the general indentation is double of what it should have been. I'll see if I can take a closer look tomorrow or sometime not too far into the future. Sounds good? |
|
The indentation is wrong because something extremely weird is going on inside Basically Could the change be caller context? Could this be some nonsense related to lexical vs dynamic scope? @lddubeau : Any ideas? |
|
Answering to my self... I sometimes hate being right: -;;; typescript-mode.el --- Major mode for editing typescript
+;;; typescript-mode.el --- Major mode for editing TypeScript -*- lexical-binding: t -*-Just for kicks I tried removing that one line diff... And then the whole test-suite runs fine again. But does that involve any risk with regard to the remaining @lddubeau : Do you have any idea? If so, I guess that means that even while keeping some of the original functions, we should still consider rewriting them to from using dynamic scope to instead use lexical scope (which I guess technically speaking is a very good thing)... If @sudarshang isn't really very well versed with Emacs Lisp, I can see how that may seem like a non-trivial task to him. I'll try to do some additional interactive testing on the simplified approach (removing the first line diff) tomorrow. |
|
@sudarshang: I've "stolen" your PR and applied some light changes here for anyone who wants to give it a test-try: https://github.com/ananthakumaran/typescript.el/tree/derived-jsmode From what I can tell, it works surprisingly well :) Edit: Now green on travis too: https://travis-ci.org/ananthakumaran/typescript.el/builds/262235950 |
|
@josteink thanks. I am testing out your branch. A side effect of deriving from js-mode is that the js-mode-hook will now run even when you open a .ts file. So for example, if |
As an immediate reaction, this seems to me to be a major problem. I don't recall ever having to deal with hooks being shared among major modes before. Moving back to the broader picture, since @josteink directed a few questions at me... I don't have any bright ideas about dealing with the issue of configuration names changes. And the one time I recall converting code to work with lexical binding, I recall reading the code carefully and fixing what needed to be fixed. Grunt work. It is not fresh in my memory though so whatever I learned then doesn't help now. And it is unlikely that I'll be able to help in any substantial manner with this PR. I'll comment if I feel I should but that's about it. My availability is such that to spend substantial time towards fixing an issue with the typescript mode or tide, the issue has to be directly impeding my day-to-day work. Avoiding duplication between js-mode and typescript-mode is in the nice-to-have category but not something that impedes my work so... |
That's fair enough, and as such, sorry to have bothered you :)
I'll have to agree 100% here. I suspect that with not too many tweaks we can get the code-reuse benefit without this behaviour, but if we can't that's really a show-stopper.
Sure. My question was if anyone had any ideas of possible risks/known problems with mixing code with lexical binding (like If there's no known problems associated with such a setup, I'd rather not rewriting perfectly fine code. @sudarshang: I'll take some more stabs at trying to remove the derived-hook behaviour later on when I have time and see where I'll get, because I honestly appreciate the effort you put into this. |
@josteink I don't think salvaging my effort should be part of the criterion if we should continue to work on this branch. Let us continue only if we think that important bugs are being fixed by it. In terms of where typescript mode should go in the future, we now have an insidious dependency on js-mode. Would it be better to factor out the code in js-mode so that there will be a common core which can be reused by different language specific modes such as TypeScript and maybe Flow in the future? Should we make a proposal along those lines to emacs-devel? In such a proposal should typescript-mode be a part of emacs or ELPA? |
Like
I'm obviously biased here, but in general the less I have to deal with emacs-devel, the happier I am. I feel that working on a module on Github is a million times more effective, and I'd rather not burdon myself with the endless mailing-list discussions, bureaucracy and bike-shedding which ensues on emacs-devel. If you would like to make such a proposal, I applaud you, but I won't make such a proposal myself. For that I simply don't have time. |
|
@sudarshang: I've worked a little more on your original commit, done some small edits, squashing, rebasing, etc to make it more cleanly merge against Basically ensured all edits are either deletes or edits, and that there are no moves of code we don't want to modify. As far as I can tell, this does it. Hope you don't mind. This will make it easier to merge against I've also added a failing test for As usual, you can try the result here: https://github.com/ananthakumaran/typescript.el/tree/derived-jsmode For the diff only, see here: https://github.com/ananthakumaran/typescript.el/compare/derived-jsmode?expand=1 All in all, I'm pretty happy about the results. It's mostly red. It mostly works as before. And it has fewer bugs. |
|
Given the amount of work already spent on the PR here, I wish I would have come up with the following observations earlier... but here we are. I started working on another indentation issue I ran into while writing code. The issue has to do with the indentation of class declarations. To solve the problem I looked to see if some already existing code could help distinguish an opening curly brace that starts the body of a class vs those that perform other functions. There's a series of At any rate the
Or I suppose an executive decision could be made that |
|
Answering myself: I figured out that the And discovered that So |
|
Given the feedback from @lddubeau, I think I'll have to revisit this issue and give it a little deeper re-consideration once I fully appreciate the consequences. I'll try to get that done as soon as I have time to spare. |
So only 2 months later... I'm back trying to live up to my promise. Yeah. I've been really busy, always had easy, small and medium tasks and efforts I felt like doing and closing, before going back to this one... I've simply been a little hesitant trying to dive deeply into this one, because of it's size. This would have been the biggest rewrite in this mode's history after all. Pretty massive. But by constantly delaying it... I think I've indirectly come to a conclusion.
I agree that's clearly not what we want or intended. But avoiding it will involve hacks. It seems @lddubeau has identified a few issues which makes this effort troublesome in other ways. Other ways which possible may lead us to emacs-devel. Thinking about the effort put in so far, I'd love to see if we could salvage something out of it. My guess would be the new/improved font-locking. That seems like something where we re-use what's reusable from Not really a major saving. And we'll have to deal with conflicts for changes we've recently made as well, making sure they all work too. Despite all the effort put in so far, I just can't find a way to convince myself that:
Unless anyone can convince me otherwise, I think right now the best option is to cut our losses short, and just abort this effort. Sorry, @sudarshang. :( Anyone else feeling otherwise? Please do let me know. |
|
@josteink no worries. Sorry that I just ended up wasting your time with this PR. Thank you for taking the time to try and salvage this PR. |
|
On the contrary: sorry for wasting your time by letting you put so much work into it without being able to carry thru. If you have other changes you’d like to propose in the future, I really hope you will still consider submitting a new PR. Every contributor is valued and welcome here. |
|
There's another issue lurking in the future. I don't know how far into the future we're going to run into it but I'm sure we're going to run into it eventually. For JS code, the mode I use is The fontification code is one area where I said above that it is possible that a specific feature would require a syntax tree for implementation, but I can also foresee switching to a syntax tree for other reasons. Like for instance, readability and maintainability. Right now, a lot of the mode's logic works by trying to figure out what everything around It is true that anything we implement could be affected by a switch to logic dependent on a syntax tree but what would be salvaged from the PR here is fontification code, and this is the part of the mode that's most likely to be revamped if we get a syntax tree. So @josteink I'm okay with putting this to rest. |
Yup. My exact observation while reviewing #53. Closing this one then... May a new and better Typescript-mode arise one day, but not today. |
4 participants
This commit removes most of the code copy pasted from js-mode. The
TypeScript specific behavior is implemented by the methods
typescript-indent-line and typescript--font-lock-keywords.