Skip to content

Commit

Permalink
fix: remove direct references to the node_modules directory (#4333)
Browse files Browse the repository at this point in the history
* 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 #4202 · emacs-lsp/lsp-mode](#4202)
* [ts-ls: Wrong lsp-clients-typescript-server-path · Issue #4254 · emacs-lsp/lsp-mode](#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.
  • Loading branch information
ncaq authored May 26, 2024
1 parent 7db060a commit 5ed2e81
Showing 1 changed file with 28 additions and 11 deletions.
39 changes: 28 additions & 11 deletions clients/lsp-javascript.el
Original file line number Diff line number Diff line change
Expand Up @@ -790,20 +790,37 @@ name (e.g. `data' variable passed as `data' parameter)."
(when-let ((workspace (lsp-find-workspace 'ts-ls (buffer-file-name))))
(eq 'initialized (lsp--workspace-status workspace))))

(defun lsp-clients-typescript-project-ts-server-path ()
"Return the project local TS server path."
(f-join (lsp-workspace-root) "node_modules" "typescript" "lib" "tsserver.js"))
(defun lsp-clients-typescript-require-resolve (&optional dir)
"Get the location of the typescript.
Use Node.js require.
The node_modules directory structure is suspect
and should be trusted as little as possible.
If you call require in Node.js,
it should take into account the various hooks.
For example, yarn PnP.
Optional argument DIR specifies the working directory
to run the command in."
(when-let*
((default-directory (or dir default-directory))
(output
(string-trim-right
(shell-command-to-string
"node -e \"console.log(require.resolve('typescript'))\"")))
(not-empty (not (string-empty-p output))))
(f-parent output)))

(defun lsp-clients-typescript-server-path ()
"Return the TS sever path base on settings."
(cond
((and lsp-clients-typescript-prefer-use-project-ts-server
(f-exists? (lsp-clients-typescript-project-ts-server-path)))
(lsp-clients-typescript-project-ts-server-path))
(t
"Return the TS server path based on settings."
(if-let* ((use-project-ts lsp-clients-typescript-prefer-use-project-ts-server)
(server-path (lsp-clients-typescript-require-resolve))
(server-path-exist (f-exists? server-path)))
server-path
(if (memq system-type '(cygwin windows-nt ms-dos))
(f-join (f-parent (lsp-package-path 'typescript)) "node_modules" "typescript" "lib")
(f-join (f-parent (f-parent (lsp-package-path 'typescript))) "lib" "node_modules" "typescript" "lib")))))
;; The Windows environment does not recognize the top-level PATH returned by `lsp-package-path',
;; so the real PATH is returned through Node.js.
(lsp-clients-typescript-require-resolve (f-parent (lsp-package-path 'typescript)))
(lsp-package-path 'typescript))))

(lsp-register-client
(make-lsp-client :new-connection (lsp-stdio-connection (lambda ()
Expand Down

0 comments on commit 5ed2e81

Please sign in to comment.