oracle-samples / xfc

A javascript library for Cross Frame Communication
Apache License 2.0
17 stars 24 forks source link

Adding global window.onerrror event handler for Consumer iFrame and babel config. #73

Closed shriniketsarkar closed 3 years ago

shriniketsarkar commented 3 years ago

Summary

Babel Config change : Added in the useBuiltIns to the config to ensure any missing polyfil is available. Global Error event handler : Added a window.onerror event handler to the consumer iFrame to handle any script errors that are unexpected and unhandled in the code. This will ensure that the user does not see the script error thus improving on the user experience.

Additional Details

Babel Config : This change was needed because in IE11 browser while testing SMART on FHIR applications in MPages the Object.entries function was undefined. This babel config change ensures to add the necessary polyfills if the functionality is being used. Without this change SMART apps would not load in MPages when consuming the 1.10.1 version of xfc.

Global Error handler :

shriniketsarkar commented 3 years ago

@mhemesath , @divyaashritha , @foxannefoxanne Can you take a look at this PR and provide your thoughts?

jvarness commented 3 years ago

According to the mozilla docs: https://developer.mozilla.org/en-US/docs/Web/API/GlobalEventHandlers/onerror

This implementation might be hiding what the original error was to begin with. This code prevents the default error handler from executing because it's returning true, and it also doesn't log out any information besides the line number, message, and URL. Column number could be very helpful here since a lot of production JS is minified, and logging out the original error could help reveal important debugging information.

I'm also curious if window.onerror needs to be checked for existence. Cerner I believe supports IE 11+, and onerror has been a global handler since IE9.

shriniketsarkar commented 3 years ago

According to the mozilla docs: https://developer.mozilla.org/en-US/docs/Web/API/GlobalEventHandlers/onerror

This implementation might be hiding what the original error was to begin with. This code prevents the default error handler from executing because it's returning true, and it also doesn't log out any information besides the line number, message, and URL. Column number could be very helpful here since a lot of production JS is minified, and logging out the original error could help reveal important debugging information.

I'm also curious if window.onerror needs to be checked for existence. Cerner I believe supports IE 11+, and onerror has been a global handler since IE9.

Since the JS is minified the user cannot really do anything other than clicking Yes or No on the error dialog to make the error go away. By returning with a true we are preventing the error thrown from surfacing. In this particular case that is exactly why we are wanting to add this error handler. Once the error happens it will get caught here and then logged, to avoid it from surfacing for the user.

if (window && !window.onerror) {} check should take care of the existence of the onerror function before setting it.

@jvarness I have added stack trace and other details to the log.

jvarness commented 3 years ago

@shriniketsarkar thanks for adding the original error and the column number! I'm not a collaborator on the project so I can't approve the PR but I do in spirit.

shriniketsarkar commented 3 years ago

@roxjcalderon what would be the next steps to get this merged in and released ?

mhemesath commented 3 years ago

I don't believe this error handling logic should be packaged into XFC. This isn't capturing XFC specific errors, so if a consumer of XFC wants this behavior, it should package it within its own libarary.