Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(linux): use system xdg-open when available #100

Closed
wants to merge 1 commit into from
Closed

feat(linux): use system xdg-open when available #100

wants to merge 1 commit into from

Conversation

axe312ger
Copy link

xdg-open is already available in many desktop linux distributions. This PR changes the code to detect if it is available on the system and uses that one if detected.

This will fix:

  • supplied/bundled xdg-open not compatible with your Linux distribution (Did not encouter this yet but could be a potential issue)
  • when bundling your app with tools like https://github.com/zeit/pkg, it is not possible to include the xdg-open from this repo in the executable. You need to put the xdg-open file in the same directory as your project/app executable, which I'd like to avoid. Should fix Support pkg #61 for most of the users. (Related: Error with opn vercel/pkg#213)

I am aware that users on linux without xdg-open installed will still have issues, when this lib is bundled via pkg, but I think this risk is acceptable. Especially because when supplying the app via a package manager like apt, a dependency to xdg-utils should do the trick.

What do you think @sindresorhus ?

:)

@sindresorhus
Copy link
Owner

Many Linux distributions have outdated xdg-open versions missing important bug fixes. Bundling xdg-open have stability and consistency benefits. I want this package to just work out of the box for all Linux users when used with normal Node.js.

supplied/bundled xdg-open not compatible with your Linux distribution (Did not encouter this yet but could be a potential issue)

I'm not going to change anything based on potential issues.

when bundling your app with tools like zeit/pkg, it is not possible to include the xdg-open from this repo in the executable. You need to put the xdg-open file in the same directory as your project/app executable, which I'd like to avoid.

I stand by my comment in #61, I should not have to specially work around random tools. This was already marked as fixed in pkg, but it seems like the fix was not correct and additional comments on vercel/pkg#207 are ignored. Try opening a new issue on pkg about this.

@axe312ger
Copy link
Author

Alright. Understandable reasons for you package. Thank you for your quick response.

Just want to point out it is not really a bug in pkg, it is more like a inconvenience / UX problem and not that verbose documentation on pkg side.

Will move the detection into our project to use xdg-open directly when available.

@axe312ger axe312ger deleted the feat/linux-use-build-in-xdg-open branch July 31, 2018 08:08
@axe312ger
Copy link
Author

axe312ger commented Jul 31, 2018

We worked around via:

import commandExists from 'command-exists'
import execa from 'execa'
import opn from 'opn'

if (['win32', 'darwin'].includes(process.platform)) {
  await opn(oAuthURL, {
    wait: false
  })
} else {
  try {
    const xdgOpenExists = await commandExists('xdg-open')
    if (!xdgOpenExists) {
      throw new Error('xdg-open does not exist')
    }
    await execa('xdg-open', [oAuthURL])
  } catch (_) {
    log(`Unable to open your browser automatically. Please open the following URI in your browser:\n\n${oAuthURL}\n\n`)
  }
}

@zoutepopcorn
Copy link

Thanks @axe312ger

I ve put in on npm https://github.com/zoutepopcorn/opn-pkg

@explosivose
Copy link

You can also do something like this as a workaround to point to system's xdg-open

      let opnOpts: {app?: string | string[]};
      if (['win32', 'darwin'].includes(process.platform)) {
        opnOpts = {};
      } else {
        opnOpts = {app: 'xdg-open'};
      }
      return opn(`http://localhost:${port}/client`, opnOpts);

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.

Support pkg
4 participants