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.

Sign up for GitHub

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

Jump to bottom

Remove legacy CSS hot reloader #35934

Merged
merged 5 commits into from
Sep 4, 2019
Merged

Remove legacy CSS hot reloader #35934

merged 5 commits into from
Sep 4, 2019

Conversation

jsnajdr
Copy link
Member

@jsnajdr jsnajdr commented Sep 2, 2019 •
edited

After #35866, all Calypso CSS styles are built by webpack and delivered as webpack chunks. This PR removes the legacy and custom CSS hot reload code.

There's some overlap with @sirreal's #35839 which also removes part of the legacy CSS code. After rebasing it on top of this one, it will only contain modifications to the Webpack Build Monitor and nothing else.

Update:
Originally this PR also used style-loader to support hot module reloading in dev mode, but that turned out to be a very tough nut to crack. Postponing that to another PR.

@jsnajdr jsnajdr added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. Build CSS Migration labels Sep 2, 2019
@jsnajdr jsnajdr requested review from ockham, sgomes, sirreal and a team September 2, 2019 12:25
@jsnajdr jsnajdr self-assigned this Sep 2, 2019
@jsnajdr jsnajdr added this to In progress in CSS: Migrate Styles to Module Imports via automation Sep 2, 2019
@matticbot
Copy link
Contributor

Test live: https://calypso.live/?branch=remove/legacy-css-hot-reload

@matticbot
Copy link
Contributor

matticbot commented Sep 2, 2019 •
edited

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

Webpack Runtime (~43 bytes added 📈 [gzipped])

name      parsed_size           gzip_size
manifest        +71 B  (+0.0%)      +43 B  (+0.1%)

Webpack runtime for loading modules. It is included in the HTML page as an inline script. Is downloaded and parsed every time the app is loaded.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

ockham
ockham reviewed Sep 2, 2019
View reviewed changes
packages/calypso-build/webpack/sass.js Outdated
filename,
rtlEnabled: true,
} ),
new FilterWarningsPlugin( {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is to suppress warnings coming from MiniCssExtractPluginWithRTL, maybe we should also prefix it with

Suggested change
new FilterWarningsPlugin( {
! useStyleLoader && new FilterWarningsPlugin( {

?

Copy link
Contributor

Choose a reason for hiding this comment

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

Unless those warnings are maybe even gone after #35866? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that's true, the FilterWarningsPlugin should be guarded by useStyleLoader, too. And also the WebpackRtlPlugin. The whole plugin setup is completely different in dev and prod.

@jsnajdr
Copy link
Member Author

jsnajdr commented Sep 3, 2019

There are two issues with style-loader that affect development mode:

  • because style-loader bundles styles into JS code and doesn't generate a separate CSS asset, and because we load scripts at the end of the document, as opposed to loading CSS in <head>, there is a flash of unstyled content when showing the server-rendered markup. I.e., the "loading placeholder" with masterbar-colored strip and WP logo.
  • the styles are not converted to RTL if the locale is RTL-ish. That's done by WebpackRtlPlugin that creates a copy of the .css asset on the filesystem. There are no such assets in development.

Webpack CSS, why are you so hard? 🤦‍♂

@jsnajdr
Copy link
Member Author

jsnajdr commented Sep 3, 2019

I gave up on making style-loader work and removed the changes from this PR. Let's focus on that in another PR.

@jsnajdr
Copy link
Member Author

jsnajdr commented Sep 3, 2019

I already described the issues with style-loader. For the record, here's why hot reloading in mini-css-extract-plugin doesn't work for us: webpack-contrib/mini-css-extract-plugin#444

sirreal
sirreal approved these changes Sep 4, 2019
View reviewed changes
Copy link
Member

@sirreal sirreal left a comment

Choose a reason for hiding this comment

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

Nice cleanup. I tested booting the app with http and https and everything works well 👍

index.js Show resolved Hide resolved
CSS: Migrate Styles to Module Imports automation moved this from In progress to Approved Sep 4, 2019
@sirreal sirreal 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 Sep 4, 2019
@jsnajdr jsnajdr merged commit 3ab4ef6 into master Sep 4, 2019
CSS: Migrate Styles to Module Imports automation moved this from Approved to Done Sep 4, 2019
@jsnajdr jsnajdr deleted the remove/legacy-css-hot-reload branch September 4, 2019 08:32
jsnajdr added a commit that referenced this pull request Sep 4, 2019
@jsnajdr
Fixup host parameter in server.listen call
912a71f 
Broken in ESLint cleanup in #35934.
@jsnajdr jsnajdr mentioned this pull request Sep 4, 2019
Merged
jsnajdr added a commit that referenced this pull request Sep 4, 2019
@jsnajdr
Fixup host parameter in server.listen call (#35969)
a5c368f 
Broken in ESLint cleanup in #35934.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ockham ockham ockham left review comments

@sirreal sirreal sirreal approved these changes

@sgomes sgomes Awaiting requested review from sgomes

Assignees

@jsnajdr jsnajdr

Labels
Build CSS Migration
Projects
No open projects
CSS: Migrate Styles to Module Imports
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@jsnajdr @matticbot @ockham @sirreal