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

Minify CSS in production builds #1697

Merged
merged 5 commits into from
Feb 23, 2021
Merged

Conversation

dac09
Copy link
Collaborator

@dac09 dac09 commented Jan 29, 2021

Fixes #753

While working on the prerendering, I went pretty deep into this stuff! It'll be nice to close this long standing issue.

As per docs here: https://github.com/webpack-contrib/mini-css-extract-plugin#minimizing-for-production

@github-actions
Copy link

github-actions bot commented Jan 29, 2021

📦 PR Packages

Click to Show Package Download Links

https://rw-pr-redwoodjs-com.s3.amazonaws.com/1697/create-redwood-app-0.25.1-bede195.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1697/redwoodjs-api-0.25.1-bede195.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1697/redwoodjs-api-server-0.25.1-bede195.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1697/redwoodjs-auth-0.25.1-bede195.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1697/redwoodjs-cli-0.25.1-bede195.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1697/redwoodjs-core-0.25.1-bede195.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1697/redwoodjs-dev-server-0.25.1-bede195.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1697/redwoodjs-eslint-config-0.25.1-bede195.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1697/redwoodjs-eslint-plugin-redwood-0.25.1-bede195.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1697/redwoodjs-forms-0.25.1-bede195.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1697/redwoodjs-internal-0.25.1-bede195.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1697/redwoodjs-prerender-0.25.1-bede195.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1697/redwoodjs-router-0.25.1-bede195.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1697/redwoodjs-structure-0.25.1-bede195.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1697/redwoodjs-testing-0.25.1-bede195.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1697/redwoodjs-web-0.25.1-bede195.tgz

Install this PR by running yarn rw upgrade --pr 1697:0.25.1-bede195

@dac09 dac09 requested a review from peterp January 29, 2021 16:33
@thedavidprice
Copy link
Contributor

@dac09 Thank you for this!

Might this also fix #549 and #859

Seems like minification will sort it all out, yes?

@thedavidprice
Copy link
Contributor

@dac09 have you tested this via build and/or deploy yet? Attempted to take it for a spin but ran into other issues (possible bug in canary).

@thedavidprice
Copy link
Contributor

@dac09 Using the PR upgrade command, I tried this out on my local test project that I can confirm works fine on the latest canary. Something seems to be blowing up:

Screen Shot 2021-01-29 at 1 32 55 PM

Uncaught Error: Cannot find module '@redwoodjs/auth'
    at webpackMissingModule (:8910/static/js/app.chunk.js:9132)
    at Object.../node_modules/@redwoodjs/web/dist/apollo/index.js (:8910/static/js/app.chunk.js:9132)
    at __webpack_require__ (:8910/static/js/runtime-app.bundle.js:854)
    at fn (:8910/static/js/runtime-app.bundle.js:151)
    at Object.../node_modules/@redwoodjs/web/apollo/index.js (:8910/static/js/app.chunk.js:9099)
    at __webpack_require__ (:8910/static/js/runtime-app.bundle.js:854)
    at fn (:8910/static/js/runtime-app.bundle.js:151)
    at Module../src/index.js (:8910/static/js/app.chunk.js:91347)
    at __webpack_require__ (:8910/static/js/runtime-app.bundle.js:854)
    at fn (:8910/static/js/runtime-app.bundle.js:151)
    at Object.1 (:8910/static/js/app.chunk.js:91508)
    at __webpack_require__ (:8910/static/js/runtime-app.bundle.js:854)
    at checkDeferredModules (:8910/static/js/runtime-app.bundle.js:46)
    at Array.webpackJsonpCallback [as push] (:8910/static/js/runtime-app.bundle.js:33)
    at :8910/static/js/app.chunk.js:1

You mentioned something about PR package upgrade not always working. Might that be the case here?

@dac09
Copy link
Collaborator Author

dac09 commented Jan 29, 2021

Yep, verified both in dev and build locally. I don't believe the error is related to this :)

@dac09
Copy link
Collaborator Author

dac09 commented Jan 29, 2021

@dac09 Thank you for this!

Might this also fix #549 and #859

Seems like minification will sort it all out, yes?

The issues raised here would need a bit more thought and not related to minification I believe, its more that we hoist Css across the code base and combine it into a single css.

@dac09
Copy link
Collaborator Author

dac09 commented Jan 30, 2021

So I thought about it a little more, and the reason those issues are being raised are that there's an expectation that the css being loaded is as css modules. Maybe something we should think about!

@thedavidprice
Copy link
Contributor

Testing with PR Packages

The error I ran into seems to be related to the PR packages I used, specifically yarn rw upgrade --pr 1697:0.23.0-4db9c96. Heads up to anyone else who might attempt to use 'em for testing.

I did try this locally, with success, by simply adding the packages and updating Webpack.common in my local project. Shaved a 4mb Tailwind file down to 3mb.

Other CSS Concerns

So I thought about it a little more, and the reason those issues are being raised are that there's an expectation that the css being loaded is as css modules. Maybe something we should think about!

I think we should think about this, indeed.

The reason I was asking is that the minification process, although still using one file, might sort out the "order" issue when building for production. I don't know the answer.

@dac09 dac09 added this to the v0.25.0 milestone Feb 4, 2021
Copy link
Contributor

@thedavidprice thedavidprice left a comment

Choose a reason for hiding this comment

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

GTG on my end. I do want @peterp to confirm.

@dac09 please do think on those other CSS topics/issues when you have the bandwidth.

@thedavidprice thedavidprice requested review from peterp and removed request for peterp February 10, 2021 05:42
@thedavidprice thedavidprice modified the milestones: v0.25.0, v0.26.0 Feb 10, 2021
@thedavidprice thedavidprice modified the milestones: v0.25.1, next release Feb 17, 2021
@thedavidprice
Copy link
Contributor

Let's loop in @peterp on this one before merging.

@thedavidprice thedavidprice removed this from the next release milestone Feb 22, 2021
@thedavidprice thedavidprice added this to the next release milestone Feb 23, 2021
@thedavidprice thedavidprice merged commit 751055c into redwoodjs:main Feb 23, 2021
dac09 added a commit to dac09/redwood that referenced this pull request Feb 24, 2021
… fix/dm-babel-register

* 'fix/dm-babel-register' of github.com:dac09/redwood:
  Remove bundlesize dependency (redwoodjs#1844)
  upgrade execa; fix test.js cwd (redwoodjs#1846)
  Make ESLint configuration aware of "env." (redwoodjs#1827)
  Use createMany in Seed database example (redwoodjs#1776)
  Docs: Update contributor workflow to yarn rwt link (redwoodjs#1803)
  fix(page-loader): Adds types for pageLoadingContext | Fix for pageLoader during prerender (redwoodjs#1832)
  Minify CSS in production builds (redwoodjs#1697)
  New contributor workflow (redwoodjs#1792)
  Improve/template and auth setup tests (redwoodjs#1834)
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.

Determine why production CSS isn't minified
3 participants