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

Implement gzip and brotli compression in production #598

Merged
merged 5 commits into from
Nov 21, 2019

Conversation

CLTPayne
Copy link
Contributor

@CLTPayne CLTPayne commented Sep 24, 2019

Introducing gzip and brotli compression for production builds, addressing the file generation portion of #566.

Before this work:

Page Kit was relying on the built-in Fastly on-the-fly compression which favours performance over maximum smallness.

After this work:

  • Building for production will generate a gzip and brotli compressed version of all files, as well as the original bundle.js.

Demo:

Tested this new webpack configuration in the kitchen-sink application:

Production build files before:

Kitchen Sink Build Before Compression

Production build files after (no longer compressing source maps):

Kitchen Sink Build After Compression

Notes / Considerations:

  • Have tried to follow the current pattern for webpack plugin options by providing options objects where
    a) a consumer of Page Kit can extend / amend the compression settings
    b) a developer can see how the compression is configured.
  • The following is actually the default values for this compression. I felt including this explicitly would be useful for developer understanding / quick reference but realise it does mean we'd be pinning this behaviour and miss out on changes to the plugin defaults (if that's likely).
  const compressionPluginOptions = { algorithm: 'gzip', compressionOptions: { level: 9 } }
  const brotliPluginOptions = { quality: 11 }
  • Currently we generate source maps for production (@adambraimbridge I found this surprising so did some research and found some interesting opinions)
  • Keeping the original files after compression per @sjparkinson (Generate compressed assets with compression turned up to 11 #566 (comment))
  • Debated whether source maps should also be compressed and decided that anything we ship should be compressed. Perhaps there are alternative opinions and we should reduce the number of files but as the source maps are only downloaded if dev tools are open so perhaps not a concern? UPDATE - Discussed with @i-like-robots and agreed that source maps should not be compressed.
  • There is the option to set a minRatio for the compression algorithm or threshold for minimum file size to run through the algorithms but as discussed with @i-like-robots this would lead to more 'surprising' complexity in return for a very slightly quicker build time so have not implemented. UPDATE - Have set both plugins to compress ALL files by setting the minRatio: 1 so that we can guarantee the CDN will find a .br and .gz for all .js and .css files.
  • Have not edited the Storybook webpack config as feel these changes do not apply to storybook.

Testing:

  • Decided against adding further tests to the kitchen-sink build tests as they currently run against the development build. Is this compression worth adding test step up for testing the production build?
  • Currently no tests for the dotcom-page-kit-cli

@CLTPayne CLTPayne added Build tools Enhancement New feature or request Performance Making the sites built with Page Kit faster labels Sep 24, 2019
@sjparkinson
Copy link

There is the option to set a minRatio for the compression algorithm or threshold for minimum file size to run through the algorithms but as discussed with @i-like-robots this would lead to more 'surprising' complexity in return for a very slightly quicker build time so have not implemented.

Ahhhhh this makes sense now.

Brotli, there are strong arguments that you should not use it for small files, but this focuses on on-the-fly compression which we are not doing. So I think it's okay to press on with this approach.

If you'd like to pair on the change to the CDN let me know.

@CLTPayne
Copy link
Contributor Author

CLTPayne commented Sep 24, 2019

If you'd like to pair on the change to the CDN let me know.

@sjparkinson yes!

@i-like-robots changed his mind and would like to get this implemented and see the benefits so 👍to pairing on the CDN update once the manifest is picking up the compressed files (which I'm working on now).

@adambraimbridge
Copy link
Contributor

Re: CDN, Check this out for what it's worth: https://github.com/Financial-Times/ft.com-cdn/pull/573/files

@CLTPayne CLTPayne force-pushed the generate-compressed-assets branch 4 times, most recently from 4123c64 to 38b4f10 Compare October 1, 2019 11:32
@i-like-robots i-like-robots added this to the Release v0.4.0 milestone Oct 9, 2019
@i-like-robots
Copy link
Contributor

i-like-robots commented Oct 9, 2019

* Prevents source maps being compressed with gzip and brotli
* Default of plugins is minRatio of 0.8
* minRatio is calculated on Compressed Size / Original Size
* Approach agreed here:
#566 (comment)
@CLTPayne CLTPayne merged commit 539d590 into master Nov 21, 2019
@CLTPayne CLTPayne deleted the generate-compressed-assets branch November 21, 2019 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Build tools Enhancement New feature or request Performance Making the sites built with Page Kit faster
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants