withfig / fig

Public issue tracker for Fig.
https://fig.io
MIT License
2.05k stars 63 forks source link

Tabby integration slows down tab switching #798

Closed Eugeny closed 2 years ago

Eugeny commented 2 years ago

Sanity checks

Issue Details

Switching between tabs takes 6 seconds.

Fig integration uses child_process.execFile() to interact with Fig, but that's extremely slow when you're in a notarized macOS Electron application (see https://github.com/electron/electron/issues/26143).

Tabby includes a patched version of node-pty (as @tabby-gang/node-pty) that uses posix_spawn instead of fork/exec to circumvent this.

So the integration just needs to swap out child_process for it.

Environment

Fig Version: Version 1.0.54 (B359) [U.S.] 
UserShell: /bin/zsh
Bundle path: /Applications/Fig.app
Autocomplete: true
Settings.json: true
CLI installed: true
CLI tool path: /usr/local/bin/fig
Accessibility: true
Number of specs: 0
SSH Integration: false
Tmux Integration: false
Keybindings path: 
iTerm Integration: installed!
Hyper Integration: installed!
VSCode Integration: installed!
Docker Integration: false
Symlinked dotfiles: false
Only insert on tab: false
Installation Script: true
PseudoTerminal Path: 
SecureKeyboardInput: false
SecureKeyboardProcess: <none>
Current active process: zsh (1187) - /dev/ttys006
Current working directory: /Users/eugene
Current window identifier: 11164/% (org.tabby)
Path: /usr/local/opt/openjdk/bin:/Users/eugene/Library/Python/3.9/bin:/Users/eugene/.cargo/bin:/usr/local/opt/llvm/bin:/Users/eugene/.yarn/bin:/Users/eugene/.config/yarn/global/node_modules/.bin:/opt/local/bin:/opt/local/sbin:/Users/eugene/bin:/usr/local/bin:/Users/eugene/.cargo/bin:/Users/eugene/Library/Python/3.8/bin:/opt/local/bin:/opt/local/sbin:/Users/eugene/.cargo/bin:/Library/Frameworks/Python.framework/Versions/3.8/bin:/Library/Frameworks/Python.framework/Versions/3.7/bin:/Library/Frameworks/Python.framework/Versions/3.6/bin:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/Applications/VMware Fusion.app/Contents/Public:/usr/local/share/dotnet:/opt/X11/bin:~/.dotnet/tools:/Library/Apple/usr/bin:/Library/Frameworks/Mono.framework/Versions/Current/Commands:/Applications/Wireshark.app/Contents/MacOS:/Users/eugene/.fig/bin
Fig environment variables:
  - TERM_SESSION_ID=C0295D93-FBAD-4CFC-9104-3F307148B58B
  - FIG_INTEGRATION_VERSION=5
  - FIG_TERM=1
  - FIG_TERM_VERSION=3
  - FIG_ENV_VAR=1
  - FIG_CHECKED_PROMPTS=1
MacOS Version: 12.2 21D5025f
Hardware:
  - Model Name: MacBook Pro
  - Model Identifier: MacBookPro16,2
  - Chip: 
  - Cores: 4
  - Memory: 16 GB
Eugeny commented 2 years ago

Profile-20220119T091440.json.zip

mschrage commented 2 years ago

Whoops! We invoke the fig CLI using the following code in order to communicate with the macOS app:

const cp = require('child_process')

let runCommand = function (command: string, logger?: Logger) {
    logger.info(command)
    cp.exec(command, (err) => {
        if (err && logger) {
            logger.error(err)
        }
    });
}

How would you recommend incorporating the @tabby-gang/node-pty workaround?

What's strange is that we use this same code in Hyper and VSCode and haven't run into this Electron bug before.

Eugeny commented 2 years ago

Both Hyper and VSCode are using an older Electron version which in turn uses an older libuv, which itself uses posix_spawn which is fast. Recent libuv releases moved to fork/exec due to some of their use cases not working with posix_spawn which brings this slowdown to all Electron apps.

node-pty is similar really:

var pty = require('@tabby-gang/node-pty');
try {
    pty.spawn('fig', args, {})
} catch (err) {
    if (err && logger) {
        logger.error(err)
    }
}

See https://github.com/microsoft/node-pty#example-usage

Note that if you need the command output, you'll need to listen for the data event on the pty object.

Eugeny commented 2 years ago

Side note: you should publish the plugin to NPM, otherwise it's impossible for the users to uninstall it through Tabby itself.

mschrage commented 2 years ago

Thanks for the help! I'll swap this out now ^

Also appreciate for the heads up re: Hyper & VSCode, since we'll need to fix our plugins for those emulators as well.

mschrage commented 2 years ago

If you run into this before we push the next release, a temporary fix is to remove the Fig's Tabby plugin by running:

rm -rf ~/Library/Application\ Support/tabby/plugins/node_modules/tabby-plugin-fig-integration/
Eugeny commented 2 years ago

@mschrage if you push the plugin to NPM, the users would be able to remove it via the built-in plugin manager.

mschrage commented 2 years ago

@Eugeny is this true even if the plugin is sideloaded (I just create a new folder, that contains package.json and a js entrypoint)?

This approach is nice because then we can tie the plugin version to the version of Fig that is installed locally.

Eugeny commented 2 years ago

Don't you just sideload it already? I'm specifically referring to the fact that it's "installed" locally, but doesn't exist in the registry, which leads to npm (which Tabby uses for plugin management) to fail to remove it:


❯ npm uninstall tabby-plugin-fig-integration
npm ERR! code E404
npm ERR! 404 Not Found - GET https://registry.yarnpkg.com/tabby-plugin-fig-integration - Not found
npm ERR! 404 
npm ERR! 404  'tabby-plugin-fig-integration@^0.1.0' is not in the npm registry.
npm ERR! 404 You should bug the author to publish it (or use the name yourself!)
npm ERR! 404 
npm ERR! 404 Note that you can also install from a
npm ERR! 404 tarball, folder, http url, or git url.

npm ERR! A complete log of this run can be found in:
npm ERR!     /Users/eugene/.npm/_logs/2022-01-26T23_29_39_216Z-debug.log```
mschrage commented 2 years ago

Ah I see. Yes that makes sense.

mschrage commented 2 years ago

I'm trying to run yarn build after having added @tabby-gang/node-pty as a dependency.

ERROR in /Users/mschrage/p/tabby-event-demo/src/index.ts
./src/index.ts 21:27-34
[tsl] ERROR in /Users/mschrage/p/tabby-event-demo/src/index.ts(21,28)
      TS6133: 'command' is declared but its value is never read.
ts-loader-default_e3b0c44298fc1c14

ERROR in /Users/mschrage/p/tabby-event-demo/src/index.ts
./src/index.ts 21:44-50
[tsl] ERROR in /Users/mschrage/p/tabby-event-demo/src/index.ts(21,45)
      TS6133: 'logger' is declared but its value is never read.
ts-loader-default_e3b0c44298fc1c14

webpack 5.24.4 compiled with 2 errors in 2540 ms
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
❯ yarn build
yarn run v1.22.17
$ webpack --progress --color
asset index.js 61.4 KiB [emitted] (name: main) 1 related asset
asset index.d.ts 312 bytes [compared for emit]
cacheable modules 107 KiB
  modules by path ./node_modules/@tabby-gang/node-pty/lib/*.js 45.9 KiB 8 modules
  3 modules
external "@angular/core" 42 bytes [built] [code generated]
external "rxjs" 42 bytes [built] [code generated]
external "tabby-core" 42 bytes [built] [code generated]
external "tabby-terminal" 42 bytes [built] [code generated]
external "path" 42 bytes [built] [code generated]
external "net" 42 bytes [built] [code generated]
external "events" 42 bytes [built] [code generated]
external "fs" 42 bytes [built] [code generated]
external "os" 42 bytes [built] [code generated]
external "child_process" 42 bytes [built] [code generated]
external "worker_threads" 42 bytes [built] [code generated]

WARNING in ./node_modules/@tabby-gang/node-pty/lib/unixTerminal.js 33:14-48
Module not found: Error: Can't resolve '../build/Debug/pty.node' in '/Users/mschrage/p/tabby-event-demo/node_modules/@tabby-gang/node-pty/lib'
 @ ./node_modules/@tabby-gang/node-pty/lib/index.js 13:19-57
 @ ./src/index.ts 38:35-66

WARNING in ./node_modules/@tabby-gang/node-pty/lib/windowsPtyAgent.js 37:35-74
Module not found: Error: Can't resolve '../build/Release/conpty.node' in '/Users/mschrage/p/tabby-event-demo/node_modules/@tabby-gang/node-pty/lib'
 @ ./node_modules/@tabby-gang/node-pty/lib/windowsTerminal.js 22:24-52
 @ ./node_modules/@tabby-gang/node-pty/lib/index.js 10:19-63
 @ ./src/index.ts 38:35-66

WARNING in ./node_modules/@tabby-gang/node-pty/lib/windowsPtyAgent.js 41:39-76
Module not found: Error: Can't resolve '../build/Debug/conpty.node' in '/Users/mschrage/p/tabby-event-demo/node_modules/@tabby-gang/node-pty/lib'
 @ ./node_modules/@tabby-gang/node-pty/lib/windowsTerminal.js 22:24-52
 @ ./node_modules/@tabby-gang/node-pty/lib/index.js 10:19-63
 @ ./src/index.ts 38:35-66

WARNING in ./node_modules/@tabby-gang/node-pty/lib/windowsPtyAgent.js 58:39-73
Module not found: Error: Can't resolve '../build/Debug/pty.node' in '/Users/mschrage/p/tabby-event-demo/node_modules/@tabby-gang/node-pty/lib'
 @ ./node_modules/@tabby-gang/node-pty/lib/windowsTerminal.js 22:24-52
 @ ./node_modules/@tabby-gang/node-pty/lib/index.js 10:19-63
 @ ./src/index.ts 38:35-66

4 warnings have detailed information that is not shown.
Use 'stats.errorDetails: true' resp. '--stats-error-details' to show it.

ERROR in ./node_modules/@tabby-gang/node-pty/build/Release/pty.node 1:0
Module parse failed: Unexpected character '�' (1:0)
You may need an appropriate loader to handle this file type, currently no loaders are configured to process this file. See https://webpack.js.org/concepts#loaders
(Source code omitted for this binary file)
 @ ./node_modules/@tabby-gang/node-pty/lib/index.js 49:49-85
 @ ./src/index.ts 38:35-66

webpack 5.24.4 compiled with 1 error and 4 warnings in 2643 ms

Seems like an issue with my webpack configuration. Any ideas how to fix it?

mschrage commented 2 years ago

Also it looks like this is getting fixed waaay upstream. 🤞

Eugeny commented 2 years ago

You're right - sorry, it totally slipped my mind that native modules won't work in 3rd party plugins. You can still leverage the node-pty that's built into Tabby via

import { ipcRenderer } from 'electron'
ipcRenderer.send('pty:spawn', process.env.HOME + '/.fig/bin/fig', ['hook', 'keyboard-focus-changed', 'tabby', id], {})

You'll need to add electron to externals in your Webpack config.

mschrage commented 2 years ago

@Eugeny just published the npm package. Fixes should go out in the build next week.

Thanks for all your help!

mschrage commented 2 years ago

Forgot to close this issue ages ago, but this has been resolved.