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

Handle initial network connectivity #775

Merged

Conversation

whitecrownclown
Copy link
Contributor

@whitecrownclown whitecrownclown commented Feb 12, 2019

@sindresorhus ,

Hello!

This PR should fix the initial app launch issue with network connectivity and also allow for Caprine to be launched on system start/login without the need of a manual refresh. (Fixes #421)

Any feedback is welcome. Thanks!

@whitecrownclown whitecrownclown force-pushed the feat/network-connectivity branch from 1074c0c to aa02f27 Compare February 13, 2019 13:51
@CvX
Copy link
Collaborator

CvX commented Feb 13, 2019

I don’t think this is a correct way to approach connectivity problems.

  1. Reloading the page is one of the triggers of the bug described in Vibrancy issue, gray sidebar #712.
  2. User could be interacting with the app when the reload happens which isn’t great user experience.
  3. If someone’s connection keeps dropping the app would reload constantly. Not ideal either.

We have to find a different way to force Messenger to re-establish its connections.

@whitecrownclown
Copy link
Contributor Author

whitecrownclown commented Feb 13, 2019

@CvX I know it's not a great way to do it, but this PR only handles initial connectivity, before the user has a chance to interact with the app (for example, launching the app at login, when most of the time there's no internet connection and the app remains unresponsive/blank page). I'm not sure about item number 1, but 2 and 3 would definitely not apply here.

@CvX
Copy link
Collaborator

CvX commented Feb 13, 2019

Ah, my bad! 😶 In this case, you’re right, disregard 2 and 3.
And to handle 1 it might be better to do what we do when switching between standard messenger and work chat?

app.relaunch();
app.quit();

@whitecrownclown
Copy link
Contributor Author

@CvX Yes, i could make that change, the reason why i didn't go for that at first is that to the user it will look more like a bug and it also takes a bit longer to load. Is there no way we could reload the mainWindow and find a fix for #712 ?

@CvX
Copy link
Collaborator

CvX commented Feb 16, 2019

That would be ideal, but I'm afraid it might take quite a while to fix #712, since it's possibly a bug in Chromium.

I'd suggest doing a full app relaunch for now and adding a TODO comment to remind us to change it to a simple reload as soon as #712 is fixed.

@whitecrownclown
Copy link
Contributor Author

@CvX Alright, i've done exactly that. Thanks!

Copy link
Collaborator

@CvX CvX left a comment

Choose a reason for hiding this comment

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

Alright, I've tried it out! 👨‍🔬
Other than the suggested minor changes, it would also be great if we used something like p-wait-for (with an interval of ~1000ms) so it isn't so hard on the CPU. 🙂 What do you think?

source/index.ts Outdated Show resolved Hide resolved
source/index.ts Outdated Show resolved Hide resolved
source/index.ts Outdated Show resolved Hide resolved
@whitecrownclown
Copy link
Contributor Author

@CvX I've updated the PR and also used p-wait-for instead of the while statement.

source/index.ts Outdated Show resolved Hide resolved
@sindresorhus
Copy link
Owner

This results in a weird experience if you're actually offline. Then the app is just on the Dock without anything happening or showing. I think we need a timeout after 20 (?) seconds where it shows an error message to the user that they're offline, and with a "try again" button.

@whitecrownclown whitecrownclown force-pushed the feat/network-connectivity branch from 2e9bb30 to 8081b03 Compare February 17, 2019 14:57
@whitecrownclown whitecrownclown force-pushed the feat/network-connectivity branch from 8081b03 to d38759a Compare February 17, 2019 14:58
source/index.ts Outdated Show resolved Hide resolved
@sindresorhus sindresorhus requested a review from CvX February 18, 2019 05:35
@CvX
Copy link
Collaborator

CvX commented Feb 18, 2019

I've tried the this PR on Windows, and a couple of times the relaunched app's window was blank. I didn't find out the root cause, but instead I decided to try a simpler solution: wait for the connection before creating the window. Let me know what you think @whitecrownclown, @sindresorhus.

@whitecrownclown
Copy link
Contributor Author

whitecrownclown commented Feb 18, 2019

@CvX I think it's both simpler and better. No need to relaunch app/ reload window. I did notice that after this change triggering window 'activate' will throw an error so i added in a small check.

source/util.ts Outdated
@@ -33,3 +33,20 @@ export function showRestartDialog(message: string): void {
}
);
}

export function showRetryDialog(message: string): void {
Copy link
Owner

@sindresorhus sindresorhus Feb 18, 2019

Choose a reason for hiding this comment

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

This is not really a generic utility (And the method name is outdated). I would move this and https://github.com/sindresorhus/caprine/pull/775/files#diff-21e9ddd93162651bd36f6e5bbfca8460R282 into a new file to contain the logic.

So in index.js, it would just be:

(async () => {
await ensureOnline();

await app.whenReady();

...

Copy link
Collaborator

Choose a reason for hiding this comment

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

btw. these awaits don't have to be sequential, so we can change that to, e.g.:

await Promise.all([ensureOnline(), app.whenReady()]);

Copy link
Owner

Choose a reason for hiding this comment

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

@CvX There's not really any benefit in using Promise.all here as app.whenReady() doesn't do any work. It's just a shortcut for checking for the status of the app load. So the result of using Promise.all is exactly the same as before. But it's fine, we can keep it.

@sindresorhus
Copy link
Owner

@CvX Good solution!

@CvX
Copy link
Collaborator

CvX commented Feb 18, 2019

I've renamed showRetryDialog to showWaitDialog and did some minor cleanup.

@CvX
Copy link
Collaborator

CvX commented Feb 18, 2019

@sindresorhus Prettier disagrees with you 😉

@sindresorhus sindresorhus merged commit cda87df into sindresorhus:master Feb 18, 2019
@sindresorhus
Copy link
Owner

Good work everyone :)

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.

Blank page after internet activated
3 participants