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

overlay should be injected into the page only once #87

Merged
merged 2 commits into from
Mar 10, 2016

Conversation

yyx990803
Copy link
Contributor

Currently with a multi-entry setup, every entry's hot client will inject a copy of the overlay on to the page. When a compilation error is thrown, it will be sent to all these overlays regardless of which entry chunk it was associated with, therefore there will be multiple overlays open at the same time.

This PR simply ensures there will be only one overlay injected at a time.

@glenjamin
Copy link
Collaborator

I think I'd rather do this using an ID attribute on the created overlay element - so the logic is nicely contained inside the client overlay file.

It might also be worth making the overlay element creation lazy, so we're not adding an element unless there's an error.

@yyx990803
Copy link
Contributor Author

Making it lazy would conflict with the ID attribute approach though, because when the clients are evaluated on load there would be no overlay injected, so there's no way to check for that ID.

@yyx990803
Copy link
Contributor Author

Oh I see you mean checking for the id before appending...

@yyx990803
Copy link
Contributor Author

Hmm, the issue is not only about the overlay - it's the problems function being called more than necessary. With multiple entries there will also be duplicate warnings in the console.

@yyx990803
Copy link
Contributor Author

So, to clarify it a bit: for multiple clients on the same page, each client should call its own processUpdate function, because only that client has access to the modules in the same chunk; but only one client should respond to the problems and success, because the errors are sent to every client regardless of which chunk they are associated with.

@glenjamin
Copy link
Collaborator

Ah, I see. The problems reporting needs to be a singleton on the page.

Is it possible to tie the errors to the bundles they're related to? The messages that come over the socket contain the bundle name & hash. I've never used multiple bundles, so I'm not clear how that part works, or how it fits together.

@glenjamin
Copy link
Collaborator

For now I reckon the best approach will be to create a little object/function for problem/success, which creates and holds a reference to the overlay module inside it - and then to memoise this into a singleton on window.

This will avoid the double-warn in the console too.

@yyx990803
Copy link
Contributor Author

Made the reporter a singleton, also made the overlay only attached if there are errors.

if (typeof document !== 'undefined' && options.overlay) {
overlay = require('./client-overlay');
var reporter;
// the reporter needs to be a singleton on the page.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you expand this comment to say why?

Something that mentions multiple bundles will do it

LGTM aside from that

@glenjamin glenjamin merged commit 8017538 into webpack-contrib:master Mar 10, 2016
@glenjamin
Copy link
Collaborator

Released in 2.10.0

@yyx990803
Copy link
Contributor Author

👍

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