yyx990803 / launch-editor

Open file in editor from Node.js.
MIT License
585 stars 67 forks source link

fix(launch-editor): ignore output to stderr #41

Closed SamVerschueren closed 2 years ago

SamVerschueren commented 2 years ago

Hi @yyx990803 đź‘‹

I was testing the vite-plugin-inspector in StackBlitz and noticed that it prints ps: command not found. This is because currently we don't have ps available in the terminal.

image

However, we have EDITOR set to code so this plugin uses the fallback. I noticed that stderr is ignored for Windows but not for macOS or Linux. Maybe that's for a specific reason? This PR ignores the stderr stream.


As a small sidenote. To make the plugin resolve the right editor on StackBlitz even faster, we could add the following code block at the top of the guessEditor function.

if (process.versions.webcontainer) {
    return [process.env.EDITOR];
}

The time to spawn the ps command is somewhat wasted because (a) it currently always fails, and (b) StackBlitz is a controlled environment which is identical for everyone.

I could add that block in this PR if you are interested in it or leave the code as is. Whatever you feel the most comfortable with.

Kind regards Sam

SamVerschueren commented 2 years ago

In the vite-vue-plugin-inspector, we decided to add the short path for WebContainer because it makes opening files noticeably faster and nearly instant https://github.com/webfansplz/vite-plugin-vue-inspector/pull/15.

patak-dev commented 2 years ago

@sodatea since we are using StackBlitz for both Vite and Vitest docs, maybe it isn't a bad idea to have the fast path for webcontainer in launch-editor. This would also speed up clicking on the error overlay file link and it is also used in several places in Vitest UI. What do you think? Copying for reference the changeset from https://github.com/webfansplz/vite-plugin-vue-inspector/pull/15.

  // WebContainer config always wins
  if (process.versions.webcontainer)
    return [process.env.EDITOR || "code"]

  // Explicit config always wins in non-WebContainer environments
haoqunjiang commented 2 years ago

maybe it isn't a bad idea to have the fast path for webcontainer in launch-editor

Looks good to me!

Didn't expect that ps command would be missing on *nix systems…

SamVerschueren commented 2 years ago

@sodatea Added the additional check.

Didn't expect that ps command would be missing on *nix systems…

We never came to implementing ps command yet and figured it might make sense to make sure no errors bubble up to the terminal when spawning child process. Although I agree that spawning ps on *nix systems probably never fails.