privatenumber / snap-tweet

Snap a screenshot of a tweet 📸
MIT License
343 stars 19 forks source link

feat: provide an option to select chrome binary #5

Closed slurdge closed 3 years ago

slurdge commented 3 years ago

Sometimes the chrome binary cannot be found automagically by the the chrome library. This patch allows an user to manually select it. I tried to follow closely the style of the source & linted.

privatenumber commented 3 years ago

Ideally, the specific problem is addressed prior to fixing it so that:

Currently, it sounds like you're experiencing a bug in https://github.com/GoogleChrome/chrome-launcher. Do you think you can document the bug there?

slurdge commented 3 years ago

Currently, it sounds like you're experiencing a bug in https://github.com/GoogleChrome/chrome-launcher. Do you think you can document the bug there?

Hello, I believe this is not a bug in chrome-launcher. They acknowledge that sometimes, the chrome binary cannot be found (for example, on my system it's not the default browser and not in the path). So they provided with an option to manually give the path of the binary. Some people are also using chromium, which can be difficult to find but has the functionality needed for it work.

Ideally, the specific problem is addressed prior to fixing it.

I agree, I just saw an easy fix and went for it. As it's an additional option, I thought it would be mostly benign.

An alternative (provided by the chrome-launcher library) is to specify an environment variable (CHROME_PATH). But IMHO this is harder for a new user to go from "Chrome cannot be found" to a working solution, as the command line will not help in this case.

privatenumber commented 3 years ago

A PR to improve their detection functionality that considers your case might be helpful too.

What do you think about documenting CHROME_PATH in the readme under FAQ, (and CLI help, maybe) first? If people are having trouble with it, adding first-class support for it might make more sense to me.

slurdge commented 3 years ago

What do you think about documenting CHROME_PATH in the readme under FAQ, (and CLI help, maybe) first? If people are having trouble with it, adding first-class support for it might make more sense to me.

Seems fine to me. I envision the most frequent scenario is someone like me, who runs the command with npx and gets an error message. Maybe we could fix it up like this when we catch the error:

    console.log('[snap-tweet] Error:', error.message);
    if (error.code === 'ERR_LAUNCHER_NOT_INSTALLED') {
        console.log('[snap-tweet] You can override Chrome binary path with the CHROME_PATH environment variable');
    }
    process.exit(1);
privatenumber commented 3 years ago

Sounds like a good solution!

Can the message be something like: [snap-tweet] Error: Chrome could not be automatically found! Manually pass in the Chrome binary path with the CHROME_PATH environment variable: CHROME_PATH=/path/to/chrome npx snap-tweet ...

slurdge commented 3 years ago

Yes. Do you want me to have another PR or will you include the message ? I don't mind either.

privatenumber commented 3 years ago

Yes, please work on it if you can.

Thank you so much!

slurdge commented 3 years ago

Done in another PR. I'll close this one.