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.

Sign up for GitHub

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

Jump to bottom

Improve error page at loading time #1845

Merged
merged 3 commits into from
Dec 22, 2017
Merged

Improve error page at loading time #1845

merged 3 commits into from
Dec 22, 2017

Conversation

astorije
Copy link
Member

@astorije astorije commented Dec 16, 2017 •
edited
Loading

  • Display the "Reload page" instantly and not after 5 seconds
  • Remove stack trace, buggy anyway
  • Wrap the error details so it does not expand beyond boundaries (scrollbar would not show up either)
  • Do not show the slow-loading warning on error
  • Make error details selectable instead of editable
  • Label improvements
  • Wrap all script in IIFE
  • Rename script to more generic error handling
Before After
On load:
screen shot 2017-12-16 at 16 16 43
After 5 seconds:
screen shot 2017-12-16 at 16 16 47		 screen shot 2017-12-16 at 16 17 14
screen shot 2017-12-16 at 16 16 57 screen shot 2017-12-16 at 16 17 20

@astorije astorije added the Type: Bug Issues that report and PRs that solve any defects that cause unexpected behaviors. label Dec 16, 2017
@astorije astorije requested a review from xPaw December 16, 2017 21:22
astorije
astorije commented Dec 16, 2017
View reviewed changes
client/css/style.css Outdated
-moz-user-select: text;
-ms-user-select: text;
user-select: text;
cursor: text;
Copy link
Member Author

Choose a reason for hiding this comment

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

@xPaw, I'm guessing you made the pre content-editable so people could copy-paste. I have instead made it selectable, let me know if you see something wrong in that.

Copy link
Member

Choose a reason for hiding this comment

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

Instead of duplicating these rules, can you add the selector where these are defined instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

xPaw
xPaw reviewed Dec 16, 2017
View reviewed changes
client/js/loading-slow-alert.js Outdated
}
}

var loadingSlowTimeout = setTimeout(function() {
Copy link
Member

Choose a reason for hiding this comment

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

This creates a global variables (along with the function above). Wrap this entire script in an anonymous function.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

xPaw
xPaw reviewed Dec 16, 2017
View reviewed changes
client/js/loading-slow-alert.js Outdated
error = error.message + "\n\n" + error.stack + "\n\nView developer tools console for more information and a better stacktrace.";
var data = document.createElement("pre");
if (e instanceof ErrorEvent) {
data.textContent = e.error && e.error.stack ? e.error.stack : e.message;
Copy link
Member

Choose a reason for hiding this comment

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

I didn't include stacktrace because it's completely useless without sourcemap loaded in, and that only loads in with devtools open before the error happens, so it's effectively very useless and misleading.

Copy link
Member Author

Choose a reason for hiding this comment

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

Interesting. You did include error.stack and that was always undefined (at least for me), so I figured you wanted the stack trace. Want me to remove it?

Copy link
Member Author

Choose a reason for hiding this comment

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

that only loads in with devtools open before the error happens

Odd, because I'm trying everything to cause this, even loading in private browsing and such, and it always shows up. Also I have a fallback if the stack is empty.

But I get your overall point. Want me to remove it?

Copy link
Member

Choose a reason for hiding this comment

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

I don't understand how it could possibly work since sourcemaps are loaded by devtools. Looking at other error reporters, they either post-process errors on server, or load sourcemap as specified in JS themselves.

So yeah, it's better off removing it since we're not making a fully fledged error reporter here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't understand how it could possibly work since sourcemaps are loaded by devtools.

I don't know, mate, it just works 🤷‍♂️
But you'll notice that on the screenshot it shows the stacktrace of the built JS bundle, not the original source. That makes it less interesting for sure.

Will remove.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done:

screen shot 2017-12-17 at 23 53 36

Very glad you added this page overall. I had my concerns at first because of misunderstanding, but it makes the experience more pro and less frustrating 👏

@astorije astorije force-pushed the astorije/error-loading branch from 0d6303c to c70cc2f Compare December 16, 2017 23:22
xPaw
xPaw reviewed Dec 17, 2017
View reviewed changes
client/js/loading-error-handlers.js Outdated
loadingSlow.style.display = "block";
}

displayReload();
Copy link
Member

Choose a reason for hiding this comment

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

You can move this inside the if above, if there is no slow message, there won't be a reload button anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, I moved the button outside the block though.

I'm not 100% sure of why this if exists: is it because we're not taking any chances and it could not exist in case of a bug? If so then maybe we should display the button even if the text wasn't found, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't do this (at least yet) per my message above. Let me know what you think.

Copy link
Member

Choose a reason for hiding this comment

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

The timer is not cancelled when the app loads, so it just does nothing if the element doesn't exist.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, good catch, will do and add a comment about this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, thanks!

@xPaw xPaw added this to the 2.7.0 milestone Dec 17, 2017
@astorije astorije force-pushed the astorije/error-loading branch from c70cc2f to af9f58f Compare December 18, 2017 04:58
@astorije
Improve error page at loading
0ffd4d6 
- Display the "Reload page" instantly and not after 5 seconds
- Remove stack trace, buggy anyway
- Wrap the error details so it does not expand beyond boundaries (scrollbar would not show up either)
- Do not show the slow-loading warning on error
- Make zeeoe details selectable instead of editable
- Label improvements
@astorije
Wrap entire error loading script in anonymous function to avoid leaks
84db8d8
@astorije
Rename the error handlers file at loading time
f975426
@astorije astorije force-pushed the astorije/error-loading branch from af9f58f to f975426 Compare December 21, 2017 23:24
@astorije astorije merged commit 8652ca6 into master Dec 22, 2017
@astorije astorije deleted the astorije/error-loading branch December 22, 2017 00:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xPaw xPaw xPaw approved these changes

Assignees
No one assigned
Labels
Type: Bug Issues that report and PRs that solve any defects that cause unexpected behaviors.
Projects
None yet
Milestone
2.7.0
Development

Successfully merging this pull request may close these issues.
2 participants
@astorije @xPaw