-
Notifications
You must be signed in to change notification settings - Fork 110
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
Add support for indirect buffers #203
Add support for indirect buffers #203
Conversation
This looks like an amazing (and a little big) pull. I’ll try to get it tested, reviewed and provide comments over the next few days. Anyway, I have to say already now: Awesome. Glad to see you pulled through on this one! |
I was surprised that this actually worked as well as it does. I expected that we'd have to manually send changes in the buffer to the server process, but somehow everything works out of the box. The only problem is that TypeScript doesn't include non-typescript files in the project unless you manually tell it to. |
Is that something we can fix in the configure-command? |
I don't think so. I tried manually specifying the location of tsconfig.json through the |
Thanks for the PR. I haven't gone through entire patch, I just want to point out some issues that might occur
|
Good catch! I'll move it to a function.
I was worried about that, but it shouldn't be a problem as the actual major mode hooks are only executed when the indirect buffer gets opened through
I just tried it. Right now I also noticed that this approach doesn't work for org-mode documents yet as they don't use |
1a12cf0
to
753444f
Compare
could you also add a minimal vuejs/org file inside the example folder, so it would be easier to test? I have never used vuejs myself. |
753444f
to
c888fc0
Compare
I haven't had the time to properly test everything yet, but refactoring should now work in temporary and indirect buffers, and there should be no more errors in org-mode. |
It seems like we take care of rename that originates from the indirect buffer. What happens when we rename symbols in different ts file, that is also referenced by
All these issues might go away if tsserver is aware of the |
I hadn't heard of vue-ts-plugin before. It looks like it extracts the script part of the component. That would allow for cross-file refactoring and imports without a shim (you'd normally use a shim for importing .vue components and let webpack do all the compilation), but using it will probably break editing as the plugin assumes tsserver is looking at the entire file, not just the script part. The official VsCode plugin seems to solve it by bundling an actual language server, but that's way seems way out of scope. It would be much better if we could somehow associate a file with a TypeScript project without having to add it to tsconfig.json. That would still not fix cross-file renaming, but it would at least not break anything. You'd probably want to keep your components as simple as possible anyway, and some refactors could potentially break the template part of the component. |
tide.el
Outdated
|
||
(defvar tide-servers (make-hash-table :test 'equal)) | ||
(defvar tide-response-callbacks (make-hash-table :test 'equal)) | ||
|
||
(defvar tide-source-root-directory (file-name-directory (or load-file-name buffer-file-name))) | ||
(defvar tide-source-root-directory (file-name-directory (or load-file-name (tide-buffer-file-name)))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this particular case, aren't we actually interested in the buffer-file-name
(since the buffer is going to be tide.el
, not something the user is editing)?
If we're actually interested in (tide-buffer-file-name)
, this defvar
has one issue: it's introduced in the source before tide-buffer-file-name
is, meaning evaluating this statement interactively from scratch will fail.
For a tide
-developer, that's pretty annoying, not to mention something I just fixed, so I'd like for it not to get broken again immediately :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, that's what you get for carelessly find-replacing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You forgot to fix this one :)
tide.el
Outdated
@@ -194,6 +195,16 @@ above." | |||
(let ((full-path (directory-file-name (tide-project-root)))) | |||
(concat (file-name-nondirectory full-path) "-" (substring (md5 full-path) 0 10)))) | |||
|
|||
(defun tide-buffer-file-name () | |||
"Returns either the path to the currently open file. This is | |||
needed to support indirect buffers, as they don't set |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function seems to work as intended, but the docstring has some issues.
Returns either {something} ... or what?
(if (equal file (tide-buffer-file-name)) | ||
(current-buffer) | ||
(find-file-noselect file))) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is going to leave buffers lying around after we're done editing them.
I'm not saying that is wrong per se, but I'm just wondering if it's intentional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would you propose closing unneeded buffers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking around the code other places, we are pretty careless about usage of find-file-noselect
ourselves already.
I don't have any issues with this per se, and from inspecting my buffer list after working with this, I can't say I see Emacs literred with new, additional leftover buffers.
I guess you can disregard this comment. This is fine as it is.
tide.el
Outdated
(if tide-require-manual-sync | ||
`(:file | ||
,(tide-buffer-file-name) | ||
;; TODO: Set this according to a variable or an alist |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this still something which needs to be done, or is this just a leftover comment from when you worked on it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would only be needed to support TSX in a an indirect buffer. Maybe a simple variable that could be overridden in the dir-local variables would suffice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I understand (not developing React/JSX/TSX myself), our JSX/TSX-support is already a little spotty.
Not having 100% support out of the box for literate TSX is IMO not going to be a big deal, and especially if we don't have an easy way to reliably establish what kind of scripting-kind this is.
Unless anyone else objects or has a way to properly determine this, I say TS
as a default is fine for now.
tide.el
Outdated
@@ -170,11 +170,12 @@ above." | |||
(tide-def-permanent-buffer-local tide-buffer-dirty nil) | |||
(tide-def-permanent-buffer-local tide-buffer-tmp-file nil) | |||
(tide-def-permanent-buffer-local tide-active-buffer-file-name nil) | |||
(tide-def-permanent-buffer-local tide-require-manual-sync nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can tell we're not exactly leading by example in the existing code...
But not yet being fully familiar with this code, I'd really appreciate a docstring for when I do a apropos
lookup. (Like the very descriptive code-comments in tide-setup
)
Feel free to extend the tide-def-permanent-buffer-local
to add an option for such docstrings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I originally thought that we'd have to manually synchronize the buffer's contents to tsserver (through ChangeRequest), but it turns out that tsserver is smart enough to use diffs after you pass it the initial file contents, so right now that variable is only needed for setting up the buffer. I'll add docstrings so we can document that!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it turns out that tsserver is smart enough to use diffs after you pass it the initial file contents
Should this in theory also apply to flycheck
? Because flycheck
is not heaving properly in interactive editing mode, only on initial buffer-load.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but it turns out that tsserver is smart enough to use diffs after you pass it the initial file contents,
No tsserver doesn't do anything. Once we open a file using open
command, tide is supposed to keep the file in sync. Tide currently reloads the entire file see tide-sync-buffer-contents
.
Should this in theory also apply to flycheck? Because flycheck is not heaving properly in interactive editing mode, only on initial buffer-load.
if you have the config in readme (setq flycheck-check-syntax-automatically '(save mode-enabled))
, flycheck is triggered only on save and mode-enabled. You probably need to add newline to the list I guess.
tide.el
Outdated
;; buffer | ||
(when (equal buffer-file-name file) | ||
(basic-save-buffer)) | ||
(basic-save-buffer) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really appreciate comments in code-sections like this. It helps guide the intent behind the code in ways the code alone doesn't fully convey.
That said: Here the comment says saving may not work... And then immediately after the (when)
-guard... we save anyway (and potentially ending up saving twice).
This looks fishy or it needs even better documentation to explain why this isn't wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, that wasn't intended.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it would be even better to create a function with a proper name, like
(unless (tide-indirect-buffer-p file)
(basic-save-buffer))
The intent is not clear when you read the code
;; Saving won't work if the current buffer is temporary or an indirect | ||
;; buffer | ||
(when (equal buffer-file-name file) | ||
(basic-save-buffer)) | ||
(length (plist-get location :locs)))))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And here you don't save twice. Basically this is code-repetition where we introduce the possibility of inconsistencies and bugs.
Maybe this could be extracted into a helper-function to avoid such errors in the future?
(when (string-prefix-p "/dev/null/inferredProject" | ||
(tide-plist-get response :body :configFileName)) | ||
(message (format "'%s' is not part of a project, add it to the files array in tsconfig.json" | ||
(tide-buffer-file-name))))))))) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really appreciate code like this which guides the user when there's nothing we can do about to situation, not to mention the code-comments helping us decide when things like this can possibly be obsoleted it in the future.
Very nice.
Hey Robbert! I haven't had time to do very much interactive testing yet, but at least now I've looked over the code to see what kind of changes was done. All in all I have to say I really like it, and for several reasons:
That's also quite a diff, and despite what may look like lots of comment, I wasn't able to find much in terms of things I thought were wrong or needed severe changes. This is clearly 10 out of 10 for an initial contribution! Let me know what you think about my comments so far, and I can try to do some actual end-user testing too :) |
Thank you very much for the help! 'm quite new to elisp and Emacs' inner workings in general, so a little guidance comes very welcome. |
Now I've done some functional testing, and it's clearly much better than it used to be :) That said, I seem to be having some issues compared to what I expected. When diving into the typescript babel-section in And most things did work like wonders (like eldoc, highlighting, navigation, etc). Amazing stuff. Amazing improvement. Let's just say that before diving into the issues :) I'm not sure whether you are at fault here or not, but I did find some things which did not work entirely. Some of these issues are not really so bad in themselves, but they get compounded by one single issue which has potential for causing a lot of confusion: seemingly delayed Basically errors in my code are not reported as errors until I finish my editing (as in close the indirect buffer) and re-open it. As an example, adding a new variable named x at the end of let x = new Snake("boo");
x.| With the cursor at If I save the buffer, close it and re-open it, I now get the Empowered by this knowledge, I rename the variable to Do you think this is something you can fix? Is this related to your concept of manually synched buffers? Also: where did var x = {
another: 'name'
}; Well that's certainly interesting! Maybe To test out this theory I created another literate section above your's and added the following to it:
And after that I changed your (Not sure if this is your fault or not though. But just mentioning it as a finding.) Anyone have any idea what we should call "expected behaviour" in this case? With some things automatically imported and others not... This can quickly get confusing for the average org-user. That said... All in all, I'm very much for these changes and you have my full support for getting this PR merged once we get all the small details settled :) |
That's weird, I'll look into it this weekend. I haven't experienced any such issues (screencast), but I might be using the version of tsserver that comes bundled with tsc 2.5.3 instead of the one included in the tide repo. |
Sounds good. No rush. Take whatever time you need. :) |
Hey there. Any news on this one? :) |
I kind of forgot about it! I'll look into it this evening. |
2b1f481
to
ef8716a
Compare
I've fixed some minor things, but I could not reproduce your issues @josteink. Are they still happening? I did notice that it took a newline for flycheck to update when switching between blocks in the .org file as tsserver doesn't know the file's 'contents' have been changed. I think the only big remaining issue is cross file refactoring (#203 (comment)). Not sure how you'd tackle that one though. |
I'm now at work where I use windows... And while I'm not sure that matters, I can't get any literate examples to work at all now. I'm just getting lots of errors inside
I guess I can double-check if this works better on Linux when I get home, I guess. In the meantime, we did leave some comments behind on your former commits, and I can't see them having been addressed. (Simple-to-fix stuff like double-saving, etc) Could you just take a look over those comments again? :) |
ef8716a
to
570d21f
Compare
I've gotten that error before, that seems to happen when tsserver does not receive a valid filename (i.e. when Oh and sorry for that! I was sure I had already fixed those things, but I might have forgotten to push the changes. |
Just reporting in that I've now tested on Linux and that seems to work fine, I'm still having issues with flycheck though, but even if we can't figure that one out/or get it fully resolved, I'd still consider this a massive improvement. I'll see if I can figure out why this doesn't seem to work on my Windows-machine at work though. Not having proper cross-platform support would be quite a bummer. |
As far as I can diagnose this right now, it seems like this fails on Windows because the value we send in for file is null:
I guess I'll have to verify what the behaviour is on Linux, and see if there's something in our path-handling logic which acts differently there. I'd love to get this fixed before landing it proper. |
@robbert-vdh : I feel like I'm the slowest reviewer and tester on earth, but I'd like to ensure things works properly before I merge. Sorry about the delay, and I hope you're still with me. Running (an old) Emacs 24.5.1 on Ubuntu 16.04 I can reproduce the same exact same issues as I did on Windows (where I'm running 25.3.1). This leads me to believe that this is somehow dependant on other external factors (org-version?) to function 100% correctly, and that it's not tied to Windows being Windows per se. From what I can tell, it all boils down to Inspecting its implementation and trying to edebug it, I keep finding all the (org) "overlay" parts undefined. Should they be defined? I can't find them in the org source-code. Care to give me some leads? I'd hate to see this effort go wasted :) |
No problem at all, I've been very busy as well. I think you might be onto something. Then there's still the issue of cross-file refactoring writing the wrong parts of files, though I'm not sure if there's anything we can do about that without modifying tsserver. |
Good good. Make it so.
That's not a show-stopper by any means. I'm fairly certain most people will understand that this not working 100% correctly is not a shortcoming of If you can fix that org-compatibility-issue, and I can verify it working on my "boring" Windows install, I think we can consider this good to go and ready for merge! |
The only caveat is that the file has to be manually added to tsconfig.json, as the globs in the include array ignore non-typescript files. Try to read the file name org org src blocks Move tide-buffer-file-name to a function, otherwise tide will ignore file renames TODO: - Apply refactors to the correct buffer. - Check why tide-setup doesn't fire in org-mode files.
To test this you should install 'vue-mode' and run `vue-edit-indirect-at-point` inside the TypeScript code.
Source blocks on org-mode won't have their parent buffer set until after major mode hooks have fired, meaning we can't retrieve the file name until after `tide-setup` has ran.
Synchronization of buffers was already done manually, in `tide-sync-buffer-contents`.
`org-edit-src-overlay` got renamed to `org-edit-src-overlay` in org-mode 9.0, but Emacs still ships with org-mode 8.3.
570d21f
to
181184a
Compare
I've rebased and added the fix, hopefully it works now! |
That's a confirmed fix on my end. Consider it merged! Thanks for taking time to develop and contribute this awesome feature! Feel free to stay around and send us patches whenever you see something which can be improved. Contributions are always welcome, and I promise next time around I won't spend this long reviewing :) |
This allows tide to be used in e.g. template files. The only caveat is that the file has to be manually added to tsconfig.json, as the globs in the include array ignore non-typescript files (see microsoft/TypeScript#8328). Otherwise the file will be added to an inferred project with default compiler settings. This fixes #198.