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

feat(v2): introduce new minification of CSS bundle #3716

Merged
merged 15 commits into from
Nov 13, 2020
Merged

Conversation

lex111
Copy link
Contributor

@lex111 lex111 commented Nov 10, 2020

Motivation

Since we are not using tree-shaking for CSS, it is very important for us to keep final CSS bundle size as small as possible. In this PR I am using additional tools to optimize/minify the CSS bundle as best as possible:

Since this is a pretty big/deep/hard/"aggressive" minification, there is a chance of broken CSS, although it shouldn't be in normal usage. In general, to mitigate this, I also added the option for the build script (--use-old-css-minifer), which will use the old CSS minification mechanism.
But again, although there is a risk of broken CSS as a result of new minification, this should not happen in normal use cases of Docusaurus. Perhaps in more complex cases, when users may have a lot of custom CSS code, then the new minification may break something in final CSS, but it is possible that due to incorrect use of CSS. I propose to deal with such problems in a separate order to improve the new CSS minifier, which would be suitable for all our users (see new docs in cli.md).

Apart from that, I did some cleaning of the CSS styles:

  • Removed styles for the spinner in NProgress lib
  • Removed unnecessary styles in React toogle
  • And other small removals

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Benchmarks:

Before this PR:
DISABLE_VERSIONING=true yarn build => styles.css size is 99.2 kB (current)

After this PR:
DISABLE_VERSIONING=true yarn build USE_SIMPLE_CSS_MINIFIER=true => styles.css size is 98.3 kB (after CSS cleanup)

DISABLE_VERSIONING=true yarn build => styles.css size is 84.4 kB (preview)

Saved 14.8 kB or 14.9%!

Related PRs

(If this PR adds or changes functionality, please take some time to update the docs at https://github.com/facebook/docusaurus, and link to your PR here.)

@lex111 lex111 added the pr: new feature This PR adds a new API or behavior. label Nov 10, 2020
@lex111 lex111 requested a review from slorber as a code owner November 10, 2020 07:47
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Nov 10, 2020
serverConfig.output &&
serverConfig.output.filename &&
typeof serverConfig.output.filename === 'string'
serverConfig?.output &&
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know why, but TypeScript was throwing a bunch of errors related to this piece of code, so I had to use optional chaining to get rid of them.

Copy link
Collaborator

Choose a reason for hiding this comment

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

this is surprising, do you remember the TS error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tried to remove these typing changes and the package build was successful this time. Strange, so reverted these changes.

@@ -17,11 +17,9 @@ import LogPlugin from './plugins/LogPlugin';

export default function createServerConfig({
props,
minify = true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we are not doing any optimizations in the server config I removed the minify option so as not to be misleading (instead this, the default values for minify-related options are defined in the base config function)

@netlify
Copy link

netlify bot commented Nov 10, 2020

Deploy preview for docusaurus-2 ready!

Built with commit e5a3312

https://deploy-preview-3716--docusaurus-2.netlify.app

Copy link
Collaborator

@slorber slorber left a comment

Choose a reason for hiding this comment

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

  • 1 for better minification
    Was wondering if purgecss couldn't be used for this usecase instead of custom postcss logic?

Also do we want to keep the 2 modification modes over time, or is it temporary? If we want to keep it, I don't really like the name of this option, and using boolean means we can't introduce easily a 3rd option in the future

I'd rather avoid having fn args list grow, a single option object is more suitable for long term maintenance.

Note: this is likely to conflict with my i18n branch, so try to avoid refactoring too much the build/start commands in general, until i18n is merged (https://github.com/facebook/docusaurus/pull/3325/files), as I still plan to modify these files.

packages/docusaurus/bin/docusaurus.js Outdated Show resolved Hide resolved
serverConfig.output &&
serverConfig.output.filename &&
typeof serverConfig.output.filename === 'string'
serverConfig?.output &&
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is surprising, do you remember the TS error?

@@ -84,6 +84,15 @@ Compiles your site for production.
| `--bundle-analyzer` | `false` | Analyze your bundle with the [webpack bundle analyzer](https://github.com/webpack-contrib/webpack-bundle-analyzer). |
| `--out-dir` | `build` | The full path for the new output directory, relative to the current workspace. |
| `--no-minify` | `false` | Build website without minimizing JS/CSS bundles. |
| `--use-old-css-minifier` | `false` | Use only default preset cssnano for CSS minification (see info below for more details). |
Copy link
Collaborator

Choose a reason for hiding this comment

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

if we want to keep this over time, and maybe later change or add new minifier logic, what about giving a proper name to each config? like "simple" vs "advanced" or something?

boolean + the "old" term do not look very extensible to me

packages/docusaurus/src/commands/build.ts Outdated Show resolved Hide resolved
packages/docusaurus/src/webpack/client.ts Outdated Show resolved Hide resolved
packages/docusaurus/src/webpack/base.ts Show resolved Hide resolved
@lex111
Copy link
Contributor Author

lex111 commented Nov 13, 2020

@slorber I replaced the option with environment variable to minify CSS code in the simple way (as was earlier), so PR description and docs are actualized.

website/docs/cli.md Outdated Show resolved Hide resolved
@slorber slorber merged commit 487a9f9 into master Nov 13, 2020
@lex111 lex111 deleted the lex111/optimize-css branch November 13, 2020 22:58
@lex111 lex111 added this to the v2.0.0-alpha.67 milestone Nov 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: new feature This PR adds a new API or behavior.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants