-
-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
FES: Fix some issues in global error catching #4730
Conversation
46253ee
to
d6f1b20
Compare
I am not sure that the benefit of this change justifies the complexity it adds to the core code. It is not ideal to run all of the core user functions through a wrapper property. I would love to get a third opinion on this -- @lmccart would you be willing to add your thoughts? The situation is that FES global error messages do not work with Chrome when a user doesn't use a local server or the online editor. This PR fixes this behavior so that a user can open an html file in Chrome and still get proper FES messages. We encourage using the online editor and development server approaches in most documentation, but of course some amount of new users will inevitably open the html file in a browser anyways. Do you think that the added complexity in the core (this PR) is worth it to ensure that these users (Chrome with HTML file open) get proper FES messaging? |
@stalgiag I agree that a wrapper function may have gone a bit beyond just a simple enclosing in try catch block as we discussed 😅 . But there were a few more instances where the p5 library transfers control to user-code which I did not account for earlier. There are a total of 5 instances where the transfer of control happens from p5 to user-code:
The first 3 are called directly from p5 code in the normal flow of things, so there is no need for a wrapper function there ( a simple enclosing of the calls in try-catch would work, which would not affect the code complexity much). The last two are triggered upon particular events, where the user-defined function is passed to As you correctly said, it does have a negative effect on code-readability and understandability ( more-so than if using just a simple try-catch ). I am not aware what per cent of new users run p5 by double-clicking the HTML file so I am not sure if the advantages would outweigh the disadvantages here 😅 I am open to views on that. |
Hi @akshay-99 thanks for this, and @stalgiag for your consideration of code readability. This is a tough one, but I'd come down on the side of keeping the code simpler rather than trying to account for these cases. My thinking is that most beginners now use the web editor for getting started, rather than downloading the zip and trying to double click the index file. This leads me to believe that the users that are linking in p5 from CDN or using the p5 complete download are familiar enough to run a server, or will get quickly pointed in that direction via p5 documentation and/or forum. (I used to see more confusion around running a server on github issues, and that has tapered off a lot with the introduction of the web editor.) On the other hand, from a code contributor's perspective, the core code can be some of the trickiest to follow because there are so many different things happening. So anywhere we can err on the side of simplicity helps with this. Hope that sounds reasonable. |
Thanks @lmccart for your views on this. So I will close this PR now and add these details in the Limitations section of the FES contributor docs? When I was reading about this issue and finding ways to fix it, I was confused as to why this FES functionality didn't break on the web editor as well. I initially expected it to break since the origins |
Thanks for your understanding and flexibility @akshay-99! Your suggestion to add this into the Limitations section sounds good. And yes, that's correct re: the web editor as far as I understand. |
Addresses #4662
Changes:
There was an issue with global error catching in FES. Primarily the event listener for
unhandledrejection
failed to trigger in Chrome if the same-origin policy was not satisfied, or if the protocol used wasfile:///
( running p5 locally, without a local server ). This issue was due to a security policy in Chrome ( and maybe other similar browsers ) that would silently ignore the event listener if the origin of the event and the current origin didn't match.To fix this, the idea now is to enclose the calls to user-defined functions ( such as
setup
,draw
,preload
,mouseClicked
, etc) in atry-catch
block. Since these functions are called from the p5 core, the errors in them are propagated upwards and can be handled in acatch
block. Once parsed and simplified byfesErrorMonitor
, we again throw it upwards so that the original error too can appear on the console. The propagation of an error through the call stack is not affected by the security policy, even if different origins are involved ( probably because in this case it can only be seen by functions that directly led to the error )This enclosing in a try-catch block is done by using a wrapper function
fesErrorWrapper
that wraps around a call to user-defined function from p5.PR Checklist
npm run lint
passes