-
Notifications
You must be signed in to change notification settings - Fork 296
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
fix getEventSourceWrapper window on iframe #358
base: master
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## master #358 +/- ##
==========================================
- Coverage 88.63% 88.38% -0.25%
==========================================
Files 3 3
Lines 264 267 +3
Branches 76 77 +1
==========================================
+ Hits 234 236 +2
- Misses 30 31 +1
Continue to review full report at Codecov.
|
I've tested with iframes and it works good, you can have as many iframes that you want and the hot reloads styles inside and outside the iframes! |
Closing for revision in firefox |
// cache the wrapper for other entries loaded on | ||
// the same page with the same options.path | ||
window.__whmEventSourceWrapper[options.path] = EventSourceWrapper(); | ||
top.__whmEventSourceWrapper[options.path] = EventSourceWrapper(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should not keep it cached now. e.g. On this reload server is closing web socket connection
module.hot.accept() => { window.location.reload(); });
while when creating new EventSourceWrapper(); it work properly.
This PR contains a:
Motivation / Use-Case
When using iframes HMR was initializing inside each iframe, this way intead we use the parent instance of
window.__whmEventSourceWrapper
so you have no duplicate listeners and no limit on how many iframes you can have before the HMR hungs up with too many calls.Breaking Changes
None
Additional Info