fix: remove direct references to the node_modules directory #4333
Conversation
Stop reading `node_modules` directly by the file system. The problem with this direct loading is that projects without `node_modules` in the top-level directory will not work at all. For example, when I develop with aws cdk, I cut off the `cdk` or `infra` directory and create a new `package.json` and put `node_modules` there too, but in that case, the code before this modification will not work at all. I solved this problem by simply inserting an abstraction layer. Relation. * [lsp-javascript: supply correct path to tsserver for ts-ls by kiennq · Pull Request emacs-lsp#4202 · emacs-lsp/lsp-mode](emacs-lsp#4202) * [ts-ls: Wrong lsp-clients-typescript-server-path · Issue emacs-lsp#4254 · emacs-lsp/lsp-mode](emacs-lsp#4254)
In the case of Windows, etc., `shell-command-to-string` is to use a non-bash shell. To begin with, `shell-command-to-string` seems to ignore standard error output.
`lsp-clients-typescript-server-path-by-node-require` is too long.
Pros === Since Node.js require indicates the path where the file actually exists, it automatically adapts to various environments unless there are significant system changes or changes in the usage environment. Cons === There is a small overhead at first startup due to the command execution.
|
As of the first modified PR, it was not working on Windows, so I modified it and confirmed that it works on Linux (WSL2) and Windows (Emacs installed with winget). |
clients/lsp-javascript.el
Outdated
| e.g., on Windows." | ||
| (if (memq system-type '(cygwin windows-nt ms-dos)) | ||
| (lsp-clients-typescript-require-resolve (f-parent (lsp-package-path 'typescript))) | ||
| (lsp-package-path 'typescript))) |
There was a problem hiding this comment.
I need some clarification here. We were previously using (f-join (f-parent (f-parent (lsp-package-path 'typescript))) "lib" "node_modules" "typescript" "lib"), and now just (lsp-package-path 'typescript)? Am I missing something? Can you elaborate it? 🤔
There was a problem hiding this comment.
Until the following PR, (lsp-package-path 'typescript) was just called.
lsp-javascript: supply correct path to tsserver for ts-ls by kiennq · Pull Request #4202 · emacs-lsp/lsp-mode
Maybe that was a problem in the environment of the person who made this modification, so a direct directory deep dive was done to avoid that.
I honestly don't know why different environments have different directory path locations.
My suggestion this time is to just use (lsp-package-path 'typescript) by default, and only use Node.js calls that return a wide range of real PATHs in special environments.
If it is fundamentally buggy, we should fix lsp-package-path in the first place.
By this logic, the Windows-only branching on the caller side this time is also subtle, but I was puzzled as to why the TypeScirpt server did not read the PATH with tsserver.cmd at the top level and did not know how to fix it, so I used this method as a stopgap measure. I used this method as a stopgap measure.
There was a problem hiding this comment.
Interesting thing is (lsp-package-path 'typescript) gives me tsserver and not the full path. 🤔
Simplified based on code review.
| (string-trim-right | ||
| (shell-command-to-string | ||
| "node -e \"console.log(require.resolve('typescript'))\""))) | ||
| (not-empty (not (string-empty-p output)))) |
There was a problem hiding this comment.
Variable not-empty is not used. May be we can leave it as blank?
There was a problem hiding this comment.
I am aware that it is not used.
I was told that it would be better to use a form that would not proceed unless something other than nil was bound to it.
If you want to take advantage of this, I think the format before branching with (string-empty-p output) is correct.
I used (string-empty-p output) for the name, but it gave me a warning.
|
I am late in responding because my grandmother passed away and I caught a cold. My apologies. |
Oh, I'm sorry for your loss. No worries, there is no reason to rush here!
At first, I thought you were right, but it was working on my side. Can you confirm this once again? 🤔 (Although I couldn't get the completion working) |
I was having problems in a Linux environment (WSL) when writing cdk's etc. when node_modules are in a place that is not repository top level. |
|
@jcs090218 |
I'm sorry, I'm a bit busy. I will take a look when I have time! 😓 |
|
solved my issue, I think it's ok to merge. |
|
Thank you! |
…p#4333) * fix: remove direct references to the node_modules directory Stop reading `node_modules` directly by the file system. The problem with this direct loading is that projects without `node_modules` in the top-level directory will not work at all. For example, when I develop with aws cdk, I cut off the `cdk` or `infra` directory and create a new `package.json` and put `node_modules` there too, but in that case, the code before this modification will not work at all. I solved this problem by simply inserting an abstraction layer. Relation. * [lsp-javascript: supply correct path to tsserver for ts-ls by kiennq · Pull Request emacs-lsp#4202 · emacs-lsp/lsp-mode](emacs-lsp#4202) * [ts-ls: Wrong lsp-clients-typescript-server-path · Issue emacs-lsp#4254 · emacs-lsp/lsp-mode](emacs-lsp#4254) * fix(lsp-javascript): remove shell redirect when call node In the case of Windows, etc., `shell-command-to-string` is to use a non-bash shell. To begin with, `shell-command-to-string` seems to ignore standard error output. * refactor(lsp-javascript): rename function and add docs `lsp-clients-typescript-server-path-by-node-require` is too long. * fix(lsp-javascript): use Node.js require to explore Pros === Since Node.js require indicates the path where the file actually exists, it automatically adapts to various environments unless there are significant system changes or changes in the usage environment. Cons === There is a small overhead at first startup due to the command execution. * refactor: use `if-let*` and `when-let*` Simplified based on code review.
Stop reading
node_modulesdirectly by the file system. The problem with this direct loading is that projects withoutnode_modulesin the top-level directory will not work at all. For example, when I develop with aws cdk,I cut off the
cdkorinfradirectory and create a newpackage.jsonand putnode_modulesthere too, but in that case, the code before this modification will not work at all. I solved this problem by simply inserting an abstraction layer.Relation.