Make sure content is loaded when doc is ready #206
Conversation
|
Can we merge this soon? It unblocks every Next.js apps |
|
@ryanseddon Pinging you in case this is missing your notifications as you mentioned earlier. |
|
Thanks! Just getting back from holidays looking now |
| @@ -45,6 +45,7 @@ export class Frame extends Component { | |||
|
|
|||
| const doc = this.getDoc(); | |||
| if (doc && doc.readyState === 'complete') { | |||
| this.handleLoad(); | |||
There was a problem hiding this comment.
So this seems to break for everyone outside of next.js so I'm wondering what next.js is doing to have to trigger the load event manually?
There was a problem hiding this comment.
This readyState code is here to handle cross browser differences
- Chromium based browsers don't trigger onload and need the
forceUpdateto render and trigger the load event - Firefox doesn't use
readyStateand instead will update contents on iframe load
If we introduce a third call then chromium based browsers throw as it tries to render the contents before srcdoc has injected it (srcdoc is async) so there may be a timing issue here. I'm unsure why this works in next.js based apps in chromium based browsers??
Firefox works fine with as this addition is not a codepath firefox reaches
There was a problem hiding this comment.
I could definitely see this being an async timing issue. My initialContent is fairly large and includes external stylesheets like FontAwesome and stuff, which I could see causing some timing issue if it takes too long to load, especially on a page refresh where external resources might have to reload but other things might be cached and thus trigger things in a different order, but randomly on a later refresh the timing is off in the other direction and it works fine.
There was a problem hiding this comment.
My initialContent is fairly large and includes external stylesheets like FontAwesome and stuff, which I could see causing some timing issue if it takes too long to load
Interesting, do you think you could share a test case with your intitialContent?
In some further exploring it seems the addEventListener in compnentDidMount is superfluous and actually triggers a double load event in firefox.
There was a problem hiding this comment.
I would think you could just take @baptisteArno 's test case and apply my initialContent over the top of it.
getInitialState : function() {
return initialContent : `<!DOCTYPE html><html><head>
<link href="//use.fontawesome.com/releases/v5.15.1/css/all.css" rel="stylesheet" />
<link href="//fonts.googleapis.com/css?family=Open+Sans:400,300,600,700" rel="stylesheet" type="text/css" />
<base target=_blank>
</head><body style='overflow: hidden'><div></div></body></html>`
}
...
<Frame initialContent={this.state.initialContent} > .... </Frame>
You can also try using my repo https://github.com/naturalcrit/homebrewery and just update react-frame-component to see it break. Unfortunately I'm having trouble isolating a smaller version of the app because it's inherited from someone else and I don't know all the ins and outs.
There was a problem hiding this comment.
Did you mean this PR breaks for everyone not using next.js or the original issue occurs for everyone not using next.js? I can verify this PR fixes the issue for me. I'm using Chrome on node.js, not next.js.
This PR breaks, see the github action tests. It complains about the mountTarget being null as the inititalContent provided to the iframe via srcdoc hasn't rendered yet.
It might be that next.js has aggressive caching and that's why it manifests itself there more than elsewhere
There was a problem hiding this comment.
The onLoad property on the iframe.... should it be onload, lowercase? https://www.w3schools.com/jsref/event_onload.asp
There was a problem hiding this comment.
it's a react thing it'll get normalised to onload when it's written to the DOM
There was a problem hiding this comment.
Gotcha. Like className to class. Ok.
Another possibility. Seems like if a page is in cache, onLoad can potentially trigger right away, before the handler is even assigned to onLoad.
https://stackoverflow.com/questions/19917436/window-onload-not-running-on-page-refresh
There was a problem hiding this comment.
thanks seems like there could be something related there. This is issue is hard to diagnose as it hasn't been easy for me to reproduce if someone could create a codesandbox.io where it is easily recreated it would help a lot.
|
I've just pushed #207 so I could publish an alpha release on npm https://github.com/ryanseddon/react-frame-component/releases/tag/v5.2.2-alpha.0 I think this should solve the problem you're having, can you upgrade to that package and test it out in your dev env? |
|
Yep works fine on my end 👍 |
|
URL: https://homebrewery-stage.herokuapp.com/share/LP9sypnCLc8O Results: Using Chrome's Developer Tools: Network tab, a reported DOMContentLoaded of 866ms or greater resulted in a failure to load; DOMContentLoaded of ~728ms or less resulted in success. Example result - Not working: Example result - Working: |
|
@G-Ambatte let's move the conversation to #207 seems you and @calculuschild are talking about the same thing |
Fixes #192