Skip to content

fix: remove direct references to the node_modules directory #4333

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

Merged
merged 6 commits into from
May 26, 2024
Merged
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 33 additions & 11 deletions clients/lsp-javascript.el
Original file line number Diff line number Diff line change
Expand Up @@ -790,20 +790,42 @@ 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."
(let* ((default-directory (or dir default-directory))
(output
(string-trim-right
(shell-command-to-string
"node -e \"console.log(require.resolve('typescript'))\""))))
(if (string-empty-p output)
nil
(f-parent output))))

(defun lsp-clients-typescript-package-path-direct ()
"`lsp-package-path' apply `'typescript' is modified.
because the lsp server may not recognize the tsserver executable file,
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)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting thing is (lsp-package-path 'typescript) gives me tsserver and not the full path. 🤔


(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
(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")))))
(if lsp-clients-typescript-prefer-use-project-ts-server
(let ((server-path (lsp-clients-typescript-require-resolve)))
(if (f-exists? server-path)
server-path
(lsp-clients-typescript-package-path-direct)))
(lsp-clients-typescript-package-path-direct)))

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