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

[Improvement] Suggest a try-catch handler for the SOLWriter #66

Closed
teward opened this issue Aug 25, 2016 · 2 comments
Closed

[Improvement] Suggest a try-catch handler for the SOLWriter #66

teward opened this issue Aug 25, 2016 · 2 comments
Assignees
Milestone

Comments

@teward
Copy link

teward commented Aug 25, 2016

Given the issues seen in #62, and that being the call stack issue, I would like to make a suggestion for better code handling here.

We know from my experimentation that Chrome has a small call stack cap (65535). We also know that RangeErrors are thrown here.

NOTE: All lines of code extracted from Minerva were obtained from the Chrome or Firefox developer consoles while tracing the execution and error progression of the Minerva application.

This line of code, when the Array ends up having a total number of elements larger than the call stack, is what throws the RangeError that I've observed in #62 (note that I'm going to work with non-minified code hereon in this post):

c = Array.apply([], c);

We can probably improve here by adding an error handler, which then uses a more useful error message (again, not minified), which captures the very specific RangeError being seen in these cases about the call stack being exceeded, and then generating our own RangeError message which provides more useful reporting of the core issue (and though the new RangeError is uncaught intentionally so it's reported to the end user, it's got more useful data here):

try {
    c = Array.apply(p[,c);
}
catch (err): {
    if (err.name == 'RangeError' && err.message == "Maximum call stack size exceeded") {
        throw RangeError("Too many elements in SOL file to process in your current browser.");
    }
    else {
        throw err;
    }
}

This is useful for two reasons:

  1. From what I can tell, the only time RangeError: Maximum call stack size exceeded is getting thrown with the call stack error being observed is when the array length is larger than the cap in the browser. We capture the RangeError, and then redefine its error message by throwing our own RangeError, with a customized error message indicating the core problem that it is too large to be processed by Minerva in the current browser.
  2. For any and all other types of errors, they remain unhandled and unchanged; we are only implementing a change for the one specific case observed so far.

Feel free to reject this, but it may be better to print a more useful error than just "maximum call stack size exceeded" for this case.

@gmariani gmariani self-assigned this Aug 25, 2016
@gmariani
Copy link
Owner

I've added the try/catch although it looks like the bug has been fixed in Chrome 56+. This will be in v4.2.0

@gmariani gmariani added this to the 4.2.0 milestone Jan 22, 2018
gmariani added a commit that referenced this issue Jan 23, 2018
@gmariani gmariani added the fixed label Jan 23, 2018
@gmariani
Copy link
Owner

4.2.0 is now live

@gmariani gmariani mentioned this issue Jan 30, 2018
gmariani added a commit that referenced this issue Jan 30, 2018
* Initial commit for 2.0 development. Switch from ANT to webpack and Node

* Fix for issue #66

* NPM modules updated

* NPM modules updated

* NPM modules updated

* NPM modules updated

* NPM modules updated

* NPM modules updated

* NPM modules updated

* NPM modules updated

* NPM modules updated

* Added reference files

* Updated project settings

* Fix for issue #71

* Added css spinning logo

* Added gulp-rename

* Added task to build a localhost version

* Added ESLint config

* Changed some html output formatting

* Cleaned up and formatted

* ESLint formatted

* ESLint formatted, removed sypport button logic (moved to HTML), removed spinning logo logic and changed some HTML output

* Prettier formatted, updated font sizes

* Fix for #72

* Cleaned up some unused variables

* Removed node_modules from Git

* Removed node_modules from Git

* Removed node_modules from Git

* Remove node_modules from Git

* Cleaned up some linting errors

* Updated build process, and integrated to work with VS code

* 4.2.0 RC1

* 4.2.0 RC2

* Linting changes

* 4.2.0 RC3 - Removed unused css, linted more scripts, removed unused variables

* Fixed XMLView validation, enabled service-worker, removed commented code and css. Removed PT-Mono font, updated jquery-ui CSS. Also minify inline CSS

* Linting changes

* Removed unused css

* Removed spaces

* Remove unused css

* Added opengraph and re-enabled ad

* Added

* Update CDN path in all files

* 4.2.0 RC4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants