Skip to content

Conversation

mqudsi
Copy link
Contributor

@mqudsi mqudsi commented Sep 26, 2018

Instead of serially querying npm and yarn for neovim, start both as a
neovim job and then wait for a successful result from either.

See discussion in #9001.

@mqudsi
Copy link
Contributor Author

mqudsi commented Sep 26, 2018

Need to double-check how this handles errors, presuming no on_stderr means stderr is swallowed. Also need to test under Windows.

@@ -59,16 +60,34 @@ function! provider#node#can_inspect() abort
\ || s:is_minimum_version(ver, 7, 6)
endfunction


Copy link
Member

Choose a reason for hiding this comment

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

delete

@justinmk
Copy link
Member

Good idea, nice implementation, LGTM. Will merge after you get a chance to check it on Windows.

presuming no on_stderr means stderr is swallowed.

yes

@justinmk justinmk added this to the 0.3.2 milestone Sep 26, 2018
return entry_point
let s:NodeHandler = {}
let s:NodeHandler.result = ''
function! s:NodeHandler.on_stdout(job_id, data, event)
Copy link
Member

@bfredl bfredl Sep 26, 2018

Choose a reason for hiding this comment

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

You can add yarn_opts.stdout_buffered = v:true, then this function won't be needed, and self.stdout will become available in on_exit (same format as data). Analogously for stderr, if you want to check 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.

Thanks, @bfredl. I didn't know about that, perhaps the documentation at https://neovim.io/doc/user/job_control.html can be updated to reflect that.

Copy link
Member

Choose a reason for hiding this comment

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

It is mentioned at jobstart(), which links to channel.txt. Perhaps job_control.txt should be just merged with channel.txt.

@justinmk justinmk added the needs:response waiting for reply from the author label Sep 30, 2018
Instead of serially querying npm and yarn for neovim, start both as a
neovim job and then wait for a successful result from either.

See discussion in neovim#9001.
@mqudsi
Copy link
Contributor Author

mqudsi commented Oct 17, 2018

Finally got a chance to test this under Windows and it runs well. Addressed the issues pointed out in code review and added a yarn-specific optimization to try the default location before querying yarn global dir because that is much slower than npm -g root:

mqudsi@Blitzkrieg /m/d/r/neovim> hyperfine 'npm -g root' 'yarn global dir'
Benchmark #1: npm -g root

  Time (mean ± σ):     194.2 ms ±   5.4 ms    [User: 144.9 ms, System: 62.9 ms]
 
  Range (min … max):   186.4 ms … 204.2 ms
 
Benchmark #2: yarn global dir

  Time (mean ± σ):     413.8 ms ±   7.4 ms    [User: 248.3 ms, System: 163.7 ms]
 
  Range (min … max):   404.8 ms … 429.9 ms
 
Summary

'npm -g root' ran
    2.13x faster than 'yarn global dir'

@justinmk justinmk removed the needs:response waiting for reply from the author label Oct 17, 2018
@mqudsi
Copy link
Contributor Author

mqudsi commented Oct 18, 2018

Updated.

I didn't rebase anything because I'm presuming this'll be squash committed.

@justinmk justinmk merged commit 1cf50cb into neovim:master Oct 18, 2018
@justinmk justinmk changed the title Simultaneously query both npm and yarn for neovim node provider nodejs: Simultaneously query both npm and yarn Oct 18, 2018
@janlazo
Copy link
Contributor

janlazo commented Oct 19, 2018

This never passed Appveyor but no one noticed the error was silenced because of MSBuild hack for CMake when running functional tests.

[00:12:46]   [----------] Running tests from C:/projects/neovim/test/functional\provider\nodejs_spec.lua
[00:12:46]   test\functional\helpers.lua:100: Vim(let):E903: Process failed to start: no such file or directory: "yarn"
[00:12:46]   
[00:12:46]   stack traceback:
[00:12:46]   	test\functional\helpers.lua:100: in function <test\functional\helpers.lua:93>
[00:12:46]   	test\functional\helpers.lua:642: in function 'missing_provider'
[00:12:46]   	C:/projects/neovim/test/functional\provider\nodejs_spec.lua:11: in main chunk
[00:12:46]   
[00:12:46]   [----------] 0 tests from C:/projects/neovim/test/functional\provider\nodejs_spec.lua (172.00 ms total)

yarn is not installed in Appveyor by default.
npm is a shim on Windows so why use jobstart(['npm', ...?

I'll fix it in #8378 along with the other functional tests.

justinmk added a commit to justinmk/neovim that referenced this pull request Dec 30, 2018
Besides the "visible" improvements, this release features numerous
internal improvements to the UI/screen code and test infrastructure.

Numerous patches were merged from Vim, which are not mentioned below.

FEATURES:

07ad5d7 clipboard: Support custom VimL functions neovim#9304
725da1f neovim#9401 win/TUI: Improve terminal/console support
7a8dadb neovim#9077 startup: Use $XDG_CONFIG_DIRS/nvim/sysinit.vim if it exists
feec926 neovim#9299 support <cmd> mapping in more places
0653ed6 neovim#9028 diff/highlight: Show underline for low-priority CursorLine
bddcbbb signs: Add "numhl" argument neovim#9113
05f9c7c clipboard: support Wayland (neovim#9230)
14ae394 neovim#9052 TUI: add support for undercurl and underline color
4fa3492 neovim#9023 man.vim: soft (dynamic) wrap neovim#9023

API:

8b39e4e neovim#6920 API: implement object namespaces
b1aaa0a API: Implement nvim_win_set_buf() neovim#9100
8de87c7 neovim#8180 API: virtual text annotations (nvim_buf_set_virtual_text)
2b9fc9a neovim#8660 API: add nvim_buf_is_loaded()
    API: buf_get_lines, buf_line_count handle unloaded buffers
88f77c2 API: nvim_buf_get_offset_for_line
94841e5 API/UI: neovim#8221 ext_newgrid, ext_hlstate
    (use line-based rather than char-based updates)

UI

b5cfac0 neovim#8806 TUI: use BCE again more often, (smoother resizes/scrolling)
77b5e9a neovim#9315 screen: add missing status redraw when redraw_later(CLEAR) was used
5f15788 TUI: clip invalid regions on resize (neovim#8779), fixes neovim#8774
c936ae0 neovim#9193 TUI: improvements for scrolling and clearing
f204274 neovim#9143 UI: disable clearing almost everywhere
f4b2b66 neovim#9079 TUI: always use safe cursor movement after resize
d36afaf neovim#9211 ui_options: also send when starting or from OptionSet
67f80d4 TUI: Avoid reset_cursor_color in old VTE neovim#9191
e55ebae neovim#9021 don't erase screen on `:hi Normal` during startup
c5790d9 neovim#8915 TUI: Hint wrapped lines to terminals.

FIXES:

231de72 RPC: turn errors from async calls into notifications
907ad92 TUI: Restore terminal title via "title stacking" (neovim#9407)
cb76a8a genappimage: Unset $ARGV0 at invocation neovim#9376
b48efd9 neovim#9347 TUI: FreeBSD: Improve support for BSD vt console
c16529a TUI: Konsole 18.07.70 supports DECSCUSR (neovim#9364)
aec096f os/lang: use the correct LC_NUMERIC also for OS X
5fee0be provider: improve error message (neovim#9344)
3c42d7a TUI: alacritty supports set_cursor_color neovim#9353
7bff9a5 TUI: Alacritty supports DECSCUSR (neovim#9048)
57acfce macOS: infer primary language if $LANG is empty neovim#9345
bc132ae runtime/syntax: Fix highlighting of augroup contents (neovim#9328)
715fdfe neovim#9297 VimL/confirm(): Show dialog even if :silent
799d9c3 clipboard: Prefer xclip (neovim#9302)
6dae777 provider/nodejs: fix npm,yarn detection
16bc1e9 neovim#9218 channel: avoid buffering output when only terminal and no callbacks are active
72fecad neovim#8804 Fix crash in lang_init() on macOS if lang_region = NULL
d581398 ruby: detect rbenv shims for other versions (neovim#8733)
e568ac7 neovim#9123 third-party/unibilium: Fix parsing of extended capability entries
c4c74c3 jobstart(): Fix hang on non-executable cwd neovim#9204
1cf50cb provider/nodejs: Simultaneously query npm and yarn neovim#9054
6c496db undo: Fix infinite loop if undo_read_byte returns EOF neovim#2880
f8f8357 neovim#9034 'swapfile: always show dialog'

CHANGES:

c236e80 neovim#9024 --embed: wait for UI unless --headless
180b50d neovim#9248 python: 'neovim' module was renamed to 'pynvim'
2000b6a neovim#8589 VimL: Remove legacy aliases "v:errmsg", "v:shell_error", "v:this_session"
deb18a0 defaults: background=dark neovim#2894 (neovim#9205)
c1187d4 defaults: win: 'shellpipe' for cmd.exe (neovim#8827)
justinmk added a commit to justinmk/neovim that referenced this pull request Dec 31, 2018
Besides the "visible" improvements, this release features numerous
internal improvements to the UI/screen code and test infrastructure.

Numerous patches were merged from Vim, which are not mentioned below.

FEATURES:

07ad5d7 clipboard: Support custom VimL functions neovim#9304
725da1f neovim#9401 win/TUI: Improve terminal/console support
7a8dadb neovim#9077 startup: Use $XDG_CONFIG_DIRS/nvim/sysinit.vim if it exists
feec926 neovim#9299 support <cmd> mapping in more places
0653ed6 neovim#9028 diff/highlight: Show underline for low-priority CursorLine
bddcbbb signs: Add "numhl" argument neovim#9113
05f9c7c clipboard: support Wayland (neovim#9230)
14ae394 neovim#9052 TUI: add support for undercurl and underline color
4fa3492 neovim#9023 man.vim: soft (dynamic) wrap neovim#9023

API:

8b39e4e neovim#6920 API: implement object namespaces
b1aaa0a API: Implement nvim_win_set_buf() neovim#9100
8de87c7 neovim#8180 API: virtual text annotations (nvim_buf_set_virtual_text)
2b9fc9a neovim#8660 API: add nvim_buf_is_loaded()
    API: buf_get_lines, buf_line_count handle unloaded buffers
88f77c2 API: nvim_buf_get_offset_for_line
94841e5 API/UI: neovim#8221 ext_newgrid, ext_hlstate
    (use line-based rather than char-based updates)

UI

b5cfac0 neovim#8806 TUI: use BCE again more often, (smoother resizes/scrolling)
77b5e9a neovim#9315 screen: add missing status redraw when redraw_later(CLEAR) was used
5f15788 TUI: clip invalid regions on resize (neovim#8779), fixes neovim#8774
c936ae0 neovim#9193 TUI: improvements for scrolling and clearing
f204274 neovim#9143 UI: disable clearing almost everywhere
f4b2b66 neovim#9079 TUI: always use safe cursor movement after resize
d36afaf neovim#9211 ui_options: also send when starting or from OptionSet
67f80d4 TUI: Avoid reset_cursor_color in old VTE neovim#9191
e55ebae neovim#9021 don't erase screen on `:hi Normal` during startup
c5790d9 neovim#8915 TUI: Hint wrapped lines to terminals.

FIXES:

231de72 RPC: turn errors from async calls into notifications
907ad92 TUI: Restore terminal title via "title stacking" (neovim#9407)
cb76a8a genappimage: Unset $ARGV0 at invocation neovim#9376
b48efd9 neovim#9347 TUI: FreeBSD: Improve support for BSD vt console
c16529a TUI: Konsole 18.07.70 supports DECSCUSR (neovim#9364)
aec096f os/lang: use the correct LC_NUMERIC also for OS X
5fee0be provider: improve error message (neovim#9344)
3c42d7a TUI: alacritty supports set_cursor_color neovim#9353
7bff9a5 TUI: Alacritty supports DECSCUSR (neovim#9048)
57acfce macOS: infer primary language if $LANG is empty neovim#9345
bc132ae runtime/syntax: Fix highlighting of augroup contents (neovim#9328)
715fdfe neovim#9297 VimL/confirm(): Show dialog even if :silent
799d9c3 clipboard: Prefer xclip (neovim#9302)
6dae777 provider/nodejs: fix npm,yarn detection
16bc1e9 neovim#9218 channel: avoid buffering output when only terminal and no callbacks are active
72fecad neovim#8804 Fix crash in lang_init() on macOS if lang_region = NULL
d581398 ruby: detect rbenv shims for other versions (neovim#8733)
e568ac7 neovim#9123 third-party/unibilium: Fix parsing of extended capability entries
c4c74c3 jobstart(): Fix hang on non-executable cwd neovim#9204
1cf50cb provider/nodejs: Simultaneously query npm and yarn neovim#9054
6c496db undo: Fix infinite loop if undo_read_byte returns EOF neovim#2880
f8f8357 neovim#9034 'swapfile: always show dialog'

CHANGES:

c236e80 neovim#9024 --embed: wait for UI unless --headless
180b50d neovim#9248 python: 'neovim' module was renamed to 'pynvim'
2000b6a neovim#8589 VimL: Remove legacy aliases "v:errmsg", "v:shell_error", "v:this_session"
deb18a0 defaults: background=dark neovim#2894 (neovim#9205)
c1187d4 defaults: win: 'shellpipe' for cmd.exe (neovim#8827)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants