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

Build: Async Load Happy Chat, CSS Reload, Network detection (take 2) #11868

Merged
merged 1 commit into from
Mar 24, 2017

Conversation

blowery
Copy link
Contributor

@blowery blowery commented Mar 7, 2017

This reverts commit 1008869.

A retry of #11714

Shaves 25kb off the gzip'd build bundle (424kb ➡️ 399kb). Needs more testing to verify it 1) works as expected and 2) uses a reasonable placeholder for the chat box while loading.

Most of the reason for the size diff here is that happy-chat is the only thing in boot using socket.io, which is a pretty big package itself, even after gzip. Necessary, but large.

@matticbot
Copy link
Contributor

@blowery blowery added Build [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Mar 7, 2017
@matticbot matticbot added the [Size] S Small sized issue label Mar 7, 2017
@blowery blowery force-pushed the revert/revert/async-load-happy-chat branch from 72e6baa to 6241acb Compare March 15, 2017 15:18
@blowery blowery force-pushed the revert/revert/async-load-happy-chat branch from 6241acb to 9d09ec7 Compare March 21, 2017 19:40
@blowery blowery requested a review from gziolo March 21, 2017 19:45
@Tug
Copy link
Contributor

Tug commented Mar 23, 2017

Seems to be working properly 👍
I can see that the bundle size is reduced and that async-load-components-happychat.hash.js, which contains socket.io, loads asynchronously when isChatOpen is flipped to true.

Can I have some background info on the reason it was reverted?

@Tug Tug added [Status] Needs Author Reply and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Mar 23, 2017
@blowery
Copy link
Contributor Author

blowery commented Mar 23, 2017

Can I have some background info on the reason it was reverted?

Splitting the bundle also ends up duplicating the flux store that backs HappyChat. I had thought that this duplication led to two instances of the flux store being active at the same time, but that fear turned out to be incorrect. Webpack registers modules by ID and the first one registered wins. Any subsequent module with the same ID is just ignored.

@blowery blowery added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] Needs Author Reply labels Mar 23, 2017
@matticbot matticbot added the [Size] S Small sized issue label Mar 24, 2017
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

LGTM 🚢

@gziolo gziolo added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Mar 24, 2017
@blowery blowery merged commit 9887b64 into master Mar 24, 2017
@blowery blowery deleted the revert/revert/async-load-happy-chat branch March 24, 2017 18:52
@matticbot matticbot added the [Size] S Small sized issue label Mar 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants