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

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

Conversation

shriniketsarkar
Copy link

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 :

  • Scenario : If xfc is used in a nested iFrame architecture (as in SMART apps in MPages) we have 2 instances of xfc being used for performing the security handshake. When the inner iframe is loading/framing a 3rd Party application it results in sending postMessage to the outer iFrame for resizing, etc. In such a workflow if the outer iFrame is reset/reloaded/cleared during a tab sorting event in the other iframe's container it results in reloading of the outer iFrame without methodically clearing/unloading the nested inner iFrame. If this behavior happens fast enough we have observed that it results into a race condition where several messages are being sent from the nested iFrame while the other iFrame is still reloading resulting into script issues. Since we do not have any control over when the outer iFrame will get cleared we cannot initiate a cleanup before this happens. This results in the user seeing a script error that they have to click through until the application loads.

  • Script Errors : The script errors observed in such a race condition are Date is undefined, Object expected., parseInt is undefined, resolve is undefined on a Promise, etc.

  • Consideration : Since we know that the last queued reset/clearing operation in the user's workflow will result in a successful load of the application being framed we can suppress the script errors for a better user experience.

  • Solution : Adding a global event handler using window.onerror catches such unhandled errors and logs them to the console.

@shriniketsarkar
Copy link
Author

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

@jvarness
Copy link
Contributor

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.

/**
* This code snippet handles any un-handled errors from occuring in the consumer iframe.
* This is a global event handler that will capture all the error messages and log them to the console.
* Once the error is logged to the console this onerror event handler will suppress the script error from surfacing on the UI,
Copy link
Contributor

Choose a reason for hiding this comment

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

script error from surfacing on the UI

I'd like to know more about this. What were you seeing surface?

Copy link
Author

Choose a reason for hiding this comment

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

So the script error that you would see would be like the following :
MPagesScriptIssue_Error
I dont have screen shots for examples where Date() is undefined etc(since they are really hard to recreate), but for the most part when the JS is minified you would see the Object expected error with not much detail on it as shown above.

Copy link
Author

Choose a reason for hiding this comment

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

So if the iFrames are constantly reloading when components are moved around it will result in a barrage of such error popups one after another until the reloading is complete.


/* eslint no-console: 0 */

if (window && !window.onerror) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there an example of errors being thrown? I am curious replicate this in our consuming app.

Copy link
Author

@shriniketsarkar shriniketsarkar Jun 29, 2021

Choose a reason for hiding this comment

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

I have not been able to recreate the error where only a single instance of xfc is used. This happens in the nested setting when you have xfc being consumed at the top level as well as its used to frame another 3rd party application inside the nested iFrame. That would mean the Provider of the outer frame loads another Consumer for the inner iframe.

The error thrown would be incase of the minified JS:
Object expected

Copy link
Author

Choose a reason for hiding this comment

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

@foxannefoxanne I created a project to simulate some of how SMART uses xfc here : https://github.com/shriniketsarkar/xfc/tree/simulate_multiple_frames_sortable_with_error_log

Once you clone the project and run it using npm run dev you can navigate to : http://0.0.0.0:8080/example/embedded_app_lifecycle/2_c_m_r_index.html and click the ToggleFrames button to generate the error log. I am simply generating an error by accessing a function that does not exist to simulate the log.

In this example the SMART app is just a text blob but the use of xfc is similar to how SMART uses it.
Screen Shot 2021-06-30 at 9 15 58 AM

@shriniketsarkar
Copy link
Author

shriniketsarkar commented Jun 30, 2021

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
Copy link
Contributor

@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.

roxjcalderon
roxjcalderon previously approved these changes Jul 2, 2021
@shriniketsarkar
Copy link
Author

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

@mhemesath
Copy link
Contributor

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.

@roxjcalderon roxjcalderon dismissed their stale review July 6, 2021 16:44

Mike's comment above

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.

4 participants