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

Log shell.openExternal errors #8246

Merged
merged 7 commits into from Oct 1, 2019
Merged

Log shell.openExternal errors #8246

merged 7 commits into from Oct 1, 2019

Conversation

kuychaco
Copy link
Contributor

@kuychaco kuychaco commented Sep 11, 2019

Overview

This is a follow-on PR to #8220, which had closed #8156

Description

Electron's shell.openExternal changed to become async so in order to ensure that errors don't get lost in the async void we need to explicitly catch them.

I'm choosing to log them rather than throw because surfacing the error to the user doesn't add any value. They'll notice that something is wrong by virtue of the fact that their browser didn't open correctly.

The logging will be helpful for us developers when we're debugging what went wrong and save us time identifying the root cause.

Release notes

Notes: no-notes

Electron's shell.openExternal changed to become async so in order to ensure that errors don't get lost in the async void we need to explicitly catch them.

I'm choosing to log them rather than throw because surfacing the error to the user doesn't add any value. They'll notice that something is wrong by virtue of the fact that their browser didn't open correctly.

The logging will be helpful for us developers when we're trying to debug what went wrong.
@kuychaco kuychaco added the ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Sep 11, 2019
@kuychaco kuychaco added this to Needs to be Prioritized in PR Priority via automation Sep 11, 2019
@kuychaco kuychaco added this to In Progress in Desktop 2.2 release via automation Sep 11, 2019
@kuychaco kuychaco moved this from Needs to be Prioritized to ⭐️ PR Reviewable Priority ⭐️ in PR Priority Sep 11, 2019
@kuychaco kuychaco moved this from In Progress to Awaiting Review in Desktop 2.2 release Sep 11, 2019
PR Priority automation moved this from ⭐️ PR Reviewable Priority ⭐️ to Review in progress Sep 11, 2019
Copy link
Member

@niik niik left a comment

Choose a reason for hiding this comment

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

Just a quick note about the invocation of our logging methods. Out of curiosity, can you give an example of where this method would throw? I don't believe I've ever seen that before.

app/src/main-process/menu/build-default-menu.ts Outdated Show resolved Hide resolved
@niik niik self-assigned this Sep 11, 2019
@outofambit
Copy link
Contributor

took a quick pass at this to get it across the line after discussing briefly with @kuychaco on friday. i had misunderstood

Electron's shell.openExternal changed to become async so in order to ensure that errors don't get lost in the async void we need to explicitly catch them.

PR Priority automation moved this from Review in progress to Awaiting Merge Oct 1, 2019
@niik niik merged commit dd78e03 into development Oct 1, 2019
PR Priority automation moved this from Awaiting Merge to Done Oct 1, 2019
Desktop 2.2 release automation moved this from Awaiting Review to PRs for Next Beta Oct 1, 2019
@niik niik deleted the ku-log-openExternal-errors branch October 1, 2019 13:44
@billygriffin billygriffin moved this from PRs for Next Beta to Done in Desktop 2.2 release Oct 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Fix shell.openExternal calls now that it's an async method
4 participants