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

Fixes thrown exceptions in apps using postMessage #305

Merged
merged 1 commit into from
Sep 22, 2017

Conversation

jacwright
Copy link
Contributor

My app uses postMessage for it's own purposes. This fixes an error that occurs when the appcache updates choke on messages from the application.

My app uses postMessage for it's own purposes. This fixes an error that occurs when the appcache updates choke on messages from the application.
@NekR
Copy link
Owner

NekR commented Sep 21, 2017

Hi @jacwright, seems like a right thing to do, but interesting why it passes this check:

https://github.com/NekR/offline-plugin/blob/master/tpls/runtime-template.js#L144

@jacwright
Copy link
Contributor Author

You have a good point. So I looked into it a bit more and found that FullStory (which my app uses) sends messages to each iframe to try and "record" it. The message coming through is one from full-story which doesn't match what the appcache expects. Definitely an edge case, but I suppose still nice to make it more robust against error.

@NekR
Copy link
Owner

NekR commented Sep 22, 2017

I see. But AppCache's iframe is the one sending messages and main window is accepting them. It checks and accepts messages from AppCache's iframe only. So the only situation where you fix may help is if AppCache's iframe will be sending incorrect data (which is unexpected of course). Are you sure that's what happens? Also, are you sure the proposed fix fixes your situation?

@jacwright
Copy link
Contributor Author

  1. all this fix does is a null-check, which fixes my error because I'm getting them thrown in my console
  2. FullStory injects its script into the iframe and sends messages up to the main window where it is collating all changes to the DOM in and out of iframes. This is a feature of FullStory (not one you can turn on/off) that lets it record all changes to the page, even within iframes that aren't sandboxed or that also have fullstory loaded within them. It's quite nice.

To be able to continue to use FullStory and offline-plugin I need this null check added. It is good defensive programming and will strengthen the project.

@NekR
Copy link
Owner

NekR commented Sep 22, 2017

@jacwright I'm not trying to not merge this code. Just trying to fully understand why it's needed. Now I do.

There is no hard in it indeed, but next time someone asks why it's there it's good to have this conversation around :-) Thanks for explanation!

@NekR NekR merged commit c5a46fe into NekR:master Sep 22, 2017
@NekR
Copy link
Owner

NekR commented Sep 22, 2017

Oh, I'll make npm release a bit later today.

@jacwright jacwright deleted the patch-1 branch September 22, 2017 17:01
@NekR
Copy link
Owner

NekR commented Sep 23, 2017

Sorry, I forgot to make a release yesterday, released now!

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.

None yet

2 participants