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: Drop cssFilename option from root webpack config #32195

Merged
merged 4 commits into from
Apr 16, 2019

Conversation

ockham
Copy link
Contributor

@ockham ockham commented Apr 11, 2019

Changes proposed in this Pull Request

Remove the cssFilename option from the Calypso root webpack config, inferring its value from outputFilename instead; dito for cssChunkFilename. For more background and a rationale, see #32194, which does the same for calypso-build's webpack config.

This also requires some re-arranging of filename logic in the webpack config.

There is one behavioral change: In production, CSS filenames previously followed the [name].[chunkhash].css pattern. Since they're now modeled after outputFilename, they now include a min particle, i.e. [name].[chunkhash].min.css. AFAICS, that shouldn't really matter much though.

Testing instructions

npm run distclean && npm ci
npm start

Does Calypso start and build properly?

@ockham ockham added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. Build Packages labels Apr 11, 2019
@ockham ockham requested review from a team April 11, 2019 00:21
@ockham ockham self-assigned this Apr 11, 2019
@matticbot
Copy link
Contributor

@ockham ockham added [Status] In Progress and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Apr 11, 2019
@ockham ockham force-pushed the remove/css-filename-arg-from-root-webpack-config branch from 023ee23 to 2f5f53a Compare April 11, 2019 12:01
@ockham ockham added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Apr 11, 2019
@scruffian scruffian force-pushed the remove/css-filename-arg-from-root-webpack-config branch from b2ce7e2 to 2dbfa60 Compare April 12, 2019 12:29
@scruffian
Copy link
Member

This seems to work fine, but I noticed two things:

  1. I don't see .min in the CSS file names. Maybe I'm looking in the wrong place?

  2. I get this warning in my console:

  Multiple versions of @wordpress/compose found:
    3.0.1 ./~/@wordpress/compose
    3.2.0 ./~/@wordpress/data/~/@wordpress/compose


WARNING in @wordpress/element
  Multiple versions of @wordpress/element found:
    2.1.9 ./~/@wordpress/element
    2.3.0 ./~/@wordpress/data/~/@wordpress/element


WARNING in hoist-non-react-statics
  Multiple versions of hoist-non-react-statics found:
    2.5.5 ./~/hoist-non-react-statics
    3.3.0 ./~/react-redux/~/hoist-non-react-statics


WARNING in immutable
  Multiple versions of immutable found:
    3.7.6 ./~/draft-js/~/immutable
    3.8.2 ./~/immutable

@scruffian scruffian 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 Apr 12, 2019
@ockham
Copy link
Contributor Author

ockham commented Apr 16, 2019

Thanks for rebasing @scruffian!

This seems to work fine, but I noticed two things:

  1. I don't see .min in the CSS file names. Maybe I'm looking in the wrong place?

No, that's expected -- .min is only used in production 🙂 Try NODE_ENV=production npm run build-client and check the contents of public/evergreen to verify.

  1. I get this warning in my console:
  Multiple versions of @wordpress/compose found:
    3.0.1 ./~/@wordpress/compose
    3.2.0 ./~/@wordpress/data/~/@wordpress/compose


WARNING in @wordpress/element
  Multiple versions of @wordpress/element found:
    2.1.9 ./~/@wordpress/element
    2.3.0 ./~/@wordpress/data/~/@wordpress/element


WARNING in hoist-non-react-statics
  Multiple versions of hoist-non-react-statics found:
    2.5.5 ./~/hoist-non-react-statics
    3.3.0 ./~/react-redux/~/hoist-non-react-statics


WARNING in immutable
  Multiple versions of immutable found:
    3.7.6 ./~/draft-js/~/immutable
    3.8.2 ./~/immutable

These are also present on master (we have a webpack plugin that warns about different versions of npm deps that get pulled in via different branches of the dependency tree). Safe to ignore for the purposes of this PR 😬

@ockham ockham 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 Apr 16, 2019
Copy link
Member

@scruffian scruffian left a comment

Choose a reason for hiding this comment

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

Retested. LGTM 🚢

@scruffian scruffian 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 Apr 16, 2019
@ockham ockham force-pushed the remove/css-filename-arg-from-root-webpack-config branch from b166c42 to 7d00a6a Compare April 16, 2019 11:00
@matticbot
Copy link
Contributor

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

Webpack Runtime
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.

name      parsed_size           gzip_size
manifest         +8 B  (+0.0%)       +9 B  (+0.0%)

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
Copy link
Contributor Author

ockham commented Apr 16, 2019

Thank @scruffian!

I also tested this in a local wpcalypso Docker env to make sure the changed .min.css filenames don't break things in production ✅

@ockham ockham merged commit d90bc1b into master Apr 16, 2019
@ockham ockham deleted the remove/css-filename-arg-from-root-webpack-config branch April 16, 2019 11:22
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.

3 participants