v:shell_error must also be checked immediately after the npm command.

Also should skip yarn if it was found in npm, to save time.

v:shell_error must also be checked immediately after the npm command.

Then the yarn logic needs to move inside the if statement so we don't return an empty string, right?

sure

is there not a more generic way in nodejs-land to check if a freakin' module is available? There are numerous package managers ...

is there not a more generic way in nodejs-land to check if a freakin' module is available? There are numerous package managers ...

I don't think so or I'm not aware of any universal solution for this. But also the main package managers (covers 99%) are npm & yarn

What if I do it like this instead? any drawbacks or issues?

diff --git a/runtime/autoload/provider/node.vim b/runtime/autoload/provider/node.vim
index 7d4d5698a..8b8b0f901 100644
--- a/runtime/autoload/provider/node.vim
+++ b/runtime/autoload/provider/node.vim
@@ -22,6 +22,14 @@ function! s:is_minimum_version(version, min_major, min_minor) abort
     \         && str2nr(v_list[1]) >= str2nr(a:min_minor)))
 endfunction
 
+function! s:get_node_client_entry(global_modules_path, cli_bin) abort
+  let entry_path = glob(a:global_modules_path . a:cli_bin)
+  if v:shell_error || !isdirectory(a:global_modules_path) || empty(entry_path)
+    return ''
+  endif
+  return entry_path
+endfunction
+
 " Support for --inspect-brk requires node 6.12+ or 7.6+ or 8+
 " Return 1 if it is supported
 " Return 0 otherwise
@@ -41,20 +49,13 @@ function! provider#node#Detect() abort
   if exists('g:node_host_prog')
     return g:node_host_prog
   endif
-  let npm_global_modules = get(split(system('npm root -g'), "\n"), 0, '')
-  " `yarn global dir` returns the root directory of the global modules
-  let yarn_global_modules = get(split(system('yarn global dir'), "\n"), 0, '') . '/node_modules'
-  let is_node_modules_dir = isdirectory(npm_global_modules) || isdirectory(yarn_global_modules)
-  if v:shell_error || !is_node_modules_dir
-    return ''
-  endif
   if !s:is_minimum_version(v:null, 6, 0)
     return ''
   endif
   let cli_bin = '/neovim/bin/cli.js'
-  let entry_point = empty(glob(npm_global_modules . cli_bin))
-        \ ? glob(yarn_global_modules . cli_bin)
-        \ : glob(npm_global_modules . cli_bin)
+  let entry_point = s:get_node_client_entry(get(split(system('npm root -g'), "\n"), 0, ''), cli_bin)
+        " `yarn global dir` returns the root directory of the global modules
+        \ || s:get_node_client_entry(get(split(system('yarn global dir'), "\n"), 0, '') . '/node_modules', cli_bin)
   if !filereadable(entry_point)
     return ''
   endif

Also the check for empty(glob(npm_global_modules . cli_bin)) is now missing, which changes the logic. Was that intentional?

That's already happening inside the s:get_node_client_entry function here

let entry_path = glob(a:global_modules_path . a:cli_bin)
if v:shell_error || !isdirectory(a:global_modules_path) || empty(entry_path) " <--
  return ''
endif
return entry_path

Looks good, but pass the 'npm root -g' command to s:get_node_client_entry then call system from there, so that the v:shell_error check is coupled with it more

The only reason I didn't do this was that for yarn I need to concat with /node_modules

s:get_node_client_entry(get(split(system('npm root -g'), "\n"), 0, ''), cli_bin)
" vs
s:get_node_client_entry(get(split(system('yarn global dir'), "\n"), 0, '') . '/node_modules', cli_bin)

@justinmk what do you think about this?

function! s:get_node_client_entry(package_manager_cmd) abort
  let cli_bin = '/neovim/bin/cli.js'
  " `yarn global dir` returns the root directory of the global modules
  let yarn_extra_path = a:package_manager_cmd ==# 'yarn global dir' ? '/node_modules' : ''
  let entry_path = glob(get(split(system(a:package_manager_cmd), "\n"), 0, '') . yarn_extra_path . cli_bin)
  if v:shell_error || empty(entry_path) || !isdirectory(entry_path)
    return ''
  endif
  return entry_path
endfunction

function! provider#node#Detect() abort
  if exists('g:node_host_prog')
    return g:node_host_prog
  endif
  if !s:is_minimum_version(v:null, 6, 0)
    return ''
  endif
  let entry_point = s:get_node_client_entry('npm root -g')
  let entry_point = !empty(entry_point) ? entry_point : s:get_node_client_entry('yarn global dir')
  if !filereadable(entry_point)
    return ''
  endif
  return entry_point
endfunction

Not a fan of this

let yarn_extra_path = a:package_manager_cmd ==# 'yarn global dir' ? '/node_modules' : ''

but not sure how to solve this.

That's fine.

Small nit: for such small scopes package_manager_cmd name is too verbose, cmd is enough.

Will do the changes, rebase & push then 👍

with this condition, checkhealth will warn if yarn isn't installed, even if npm is. how about:

if !executable('node') || (!executable('npm') && !executable('yarn'))

use a separate advice line:

\ ['Run in shell (if you use yarn): yarn global add neovim',

What does this mean? Either node_modules/ or bin/ is in PATH?

Maybe that was not clear from my end (& also wrong). node_modules stores the bin executables inside node_modules/.bin folder that's how you can run these binaries either globally or not.

The point of npm root -g when detecting the node.js host is that it doesn't use node_modules/.bin because those are shims. Shims don't work on Windows for node.js host because you can't pass the debug flags to node binary.

Actually I think maybe this message can be removed? 'Is the global node_modules/bin directory in $PATH?'

Because:

1. npm will symlink the executables on mac or Linux in /usr/local/bin (don't know about windows)
2. yarn you will need to add $(yarn global dir)/node_modules in your $PATH

I'd remove it. In Windows, npm will copy (ie. npm link neovim copies the node_modules/neovim/ and its dependencies).

@justinmk are you also ok with removing this line?

'Is the global node_modules/bin directory in $PATH?' or before my change 'Is the npm bin directory in $PATH?'

sure

Pass the name of the package manager instead to determine the command for the directory for global node modules. You can then check if the package manager is in PATH via executable().

We do a check for executables here already https://github.com/neovim/neovim/pull/8528/files#diff-2d7f214e0c52c9cb5f1601bdfe0e46b6R505

but also if I pass only the name then I'll need to reconstruct the command inside the function too depends on that

let package_manager = executable(a:package_manager) ? a:package_manager : ''
let cmd =  package_manager ==# 'npm' ? package_manager . ' root -g' : package_manager . ' global dir'

That's the point. It's internal to the function so that it's easier to use and modify.

That's the point. It's internal to the function so that it's easier to use and modify.

I understand, just thought passing the full command will make it more flexible. So if a new package manager X is released you pass X find-root command instead of adding more logic. But I'm fine doing this too.

It doesn't hurt to have "redundant" executable() checks for autoloaded functions because the user can test each function if they work without running :checkhealth.

You need to check if yarn global dir returns a valid path on top of its node_modules/ subfolder otherwise it might use /node_modules/, where / is root, as its entry point if v:shell_error returns 0.

The check is already here, no? if v:shell_error || !isdirectory(global_modules_dir)
https://github.com/neovim/neovim/pull/8528/files/096a99f3dee634c11843baeb0fb3cc724d81fda4#diff-f9dd34ad99ab738d7adc7261f3fbb879R31

Fixed

Can you make the code more explicit? Only npm and yarn are supported.

let cmd = a:package_manager ==# 'yarn' ? 'yarn global dir' : 'npm root -g'

Ok will do that.

@janlazo good now?

function! s:get_node_client_entry(package_manager) abort
  if !executable(a:package_manager)
    return ''
  endif
  let cli_bin = '/neovim/bin/cli.js'
  let cmd =  a:package_manager ==# 'yarn' ? 'yarn global dir' : 'npm root -g'
  let cmd_output = system(cmd)
  if v:shell_error || !isdirectory(cmd_output)
    return ''
  endif
  " `yarn global dir` returns the root directory of the global modules
  " so we need to append the `/node_modules` part
  let yarn_extra_path = a:package_manager ==# 'yarn' ? '/node_modules' : ''
  let global_modules_dir = get(split(cmd_output, "\n"), 0, '') . yarn_extra_path
  if !isdirectory(global_modules_dir)
    return ''
  endif
  let entry_path = glob(global_modules_dir . cli_bin)
  if !filereadable(entry_path)
    return ''
  endif
  return entry_path
endfunction



function! provider#node#Detect() abort
  if exists('g:node_host_prog')
    return g:node_host_prog
  endif
  if !s:is_minimum_version(v:null, 6, 0)
    return ''
  endif
  let entry_point = s:get_node_client_entry('npm')
  let entry_point = !empty(entry_point) ? entry_point : s:get_node_client_entry('yarn')
  return entry_point
endfunction

Why did you break up get(split(system(cmd), "\n"), 0, '')? You need that to remove traililng <CR> before passing the output to isdirectory.

That was a mistake, this is better now?

function! s:get_node_client_entry(package_manager) abort
  if !executable(a:package_manager)
    return ''
  endif
  let cli_bin = '/neovim/bin/cli.js'
  let cmd =  a:package_manager ==# 'yarn' ? 'yarn global dir' : 'npm root -g'
  let modules_dir = get(split(system(cmd), "\n"), 0, '')
  if v:shell_error || !isdirectory(modules_dir)
    return ''
  endif
  " `yarn global dir` returns the root directory of the global modules
  " so we need to append the `/node_modules` part
  let global_modules_dir = a:package_manager ==# 'yarn' ? modules_dir . '/node_modules' : modules_dir
  if !isdirectory(global_modules_dir)
    return ''
  endif
  let entry_path = glob(global_modules_dir . cli_bin)
  if !filereadable(entry_path)
    return ''
  endif
  return entry_path
endfunction



function! provider#node#Detect() abort
  if exists('g:node_host_prog')
    return g:node_host_prog
  endif
  if !s:is_minimum_version(v:null, 6, 0)
    return ''
  endif
  let entry_point = s:get_node_client_entry('npm')
  let entry_point = !empty(entry_point) ? entry_point : s:get_node_client_entry('yarn')
  return entry_point
endfunction

@janlazo I pushed the latest changes have a look & let me know what you think.

cli_bin is used once so why not glob(global_modules_dir . '/neovim/bin/cli.js')?

Misleading variable name because modules_dir is used already and npm's global modules directory is also its root directory. I'd reuse modules_dir variable here by appending /node_modules for yarn or rename both variables.

@janlazo check again, updated the PR.