Feature request: new `lsp-clients-flow` customize var to use the `flow` binary in `node_modules/.bin`

[isker]

The recommended way to install flow is as a non-global (i.e. project-local) npm dependency.

Since the same npm dependency implements flow's language server, the median lsp-mode flow user has to customize lsp-clients-flow-server away from the default "flow on exec-path" to point to their project-local flow, located at node_modules/.bin/flow. (I personally have no global flow on my exec-path at all, which means lsp-mode will select the typescript server as a fallback in my flow projects when this variable is not correctly configured. Not fun!)

Customizing lsp-clients-flow-server per-project effectively is surprisingly hard:

• You could use directory local variables, but this variable is not deemed safe by emacs, so further configuration is needed to make this to work. Moreover, I have personally struggled to get this to work at all: if you set the dirlocal variable in all modes, it could very well be overwritten when lsp-mode is loaded lazily; if you set it just for lsp-mode, it seems like lsp-mode selects the server before dirlocal variables are set (!?), thus the configuration has no effect until you manually restart the workspace's language server.
• You could use some external path-hacking tools like direnv and emacs-direnv to put the project-local flow on your PATH when visiting files within your project. However, a similar problem exists here as for dirlocal variables. lsp-mode launches a server before direnv-mode has a chance to update exec-path. There are various workarounds proposed, but none of them work for me :).

With all of that in mind, I think providing a new customize variable within lsp-mode itself to use the workspace-local npm flow binary could make this a lot easier.

Thanks for your consideration.

(I'm not great at emacs lisp, but if you can point me to any examples of resolving files relative to the workspace root, I think I could implement this myself.)

[yyoncho]

Customization via dir locals is supported - check https://emacs-lsp.github.io/lsp-mode/page/faq/ and I think that a variable can be marked as safe either in our code or in your config.

With all of that in mind, I think providing a new customize variable within lsp-mode itself to use the workspace-local npm flow binary could make this a lot easier.

What we can do is to do that check automatically and use the local version. But with one limitation - the project root is selected after the check if language server is present. Then in order that to work you should first add the workspace folder via M-x lsp-workspace-folders-add and then open a file.

[yyoncho]

Another option is to support placeholders in the path, e. g.:

"{workspace-root}/.npm/flowjs". Then lsp-mode will expand {workspace-root} with the correct workspace root.

[isker]

check https://emacs-lsp.github.io/lsp-mode/page/faq/

Ah, I had missed that FAQ entry. I will try it out.

I think that a variable can be marked as safe either in our code or in your config.

I ended up doing this with a giant hammer: (advice-add 'risky-local-variable-p :override #'ignore). If lsp-mode can include something better, that'd be an improvement.

the project root is selected after the check if language server is present.

Ah, I didn't know that. The main intent behind this feature request is to give us a more out-of-the-box setup. (Chasing after VSCode!) If we have to select the root beforehand, that is just another kind of configuration. Probably less complicated than the local variables setup, though 🙂 .

[isker]

If we did automatically first try to find flow in node_modules/.bin, that'd be easy for me.

But I think the workflow would be unintuitive to new users. They'd have their flow skipped for a typescript server, then have to investigate the logs to find out that they needed to specify the workspace root beforehand. Then they'd have to do that and restart the server for the workspace.

[yyoncho]

I ended up doing this with a giant hammer: (advice-add 'risky-local-variable-p :override #'ignore). If lsp-mode can include something better, that'd be an improvement.

We can mark the variable as safe.

If we have to select the root beforehand, that is just another kind of configuration. Probably less complicated than the local variables setup, though

Agree.

But I think the workflow would be unintuitive to new users. They'd have their flow skipped for a typescript server, then have to investigate the logs to find out that they needed to specify the workspace root beforehand. Then they'd have to do that and restart the server for the workspace.

That is true. In the beginning, I thought that check server present and then select root will be better because otherwise you might be asked for root and then have a notification that there is no server for you. We may revisit this decision.

[yyoncho]

That is true. In the beginning, I thought that check server present and then select root will be better because otherwise you might be asked for root and then have a notification that there is no server for you. We may revisit this decision

I think also that in the early stage of the project the idea was to use prog-mode-hook for lsp command. Then, it would have been poor experience if you got asked to select root for elisp file for example, even it is clear that there won't be a language server for it.

[chetstone]

@isker Have you tried add-node-modules-path?

[raxod502]

We could also take the approach that I do in Apheleia, and---when configured---try to locate a per-project binary via (locate-dominating-file "node_modules"), and use it if found, otherwise default to a global installation.

This would also solve the issue where node_modules is not always located at the project root.