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

Add support for Brotli compression #2273

Merged
merged 3 commits into from
Sep 18, 2019
Merged

Conversation

clupprich
Copy link
Contributor

@clupprich clupprich commented Sep 6, 2019

Adds support for Brotli compression via the compression-webpack-plugin package/plugin when running Node versions >=11.7.0, as described in #2089.

I could use some help in writing tests for this - any good starting points?
I'm also not really happy about the added dependency on the semver package - maybe some small utility function would do the trick here? Fixed that in 716ac1d by checking the presence of process.versions.brotli.

@clupprich
Copy link
Contributor Author

Ah, Brotli has its own entry in process.versions, I can probably use that. Thanks to @julianrubisch for that hint!

@guillaumebriday
Copy link
Member

Hey!

That is so cool, thank you @clupprich 👍

I think we should add configuration example and installation details in the https://github.com/rails/webpacker/blob/master/docs/deployment.md?

What do you think?

Thanks

@clupprich
Copy link
Contributor Author

@guillaumebriday Definitely! Do you think we should also add a config flag to enable/disable this? Because Brotli isn't widely supported yet by e.g. hosted solutions (Heroku, afaik), this can be overhead that people don't need.

@guillaumebriday
Copy link
Member

I think we should not add a specific config option only for Brotli even if it's not supported by some hosted solutions. These solutions will simply ignore the Brotli files.

I think, we can add sentence like that but for Brotli:

Some servers support sending precompressed versions of files with the .gz extension when they're available. For example, nginx offers a gzip_static directive.

And add a link (or steps) to install it with a warning on the minimal node version required.

What do you think?

@clupprich
Copy link
Contributor Author

Added the instructions in f70f7ef. Turns out the hard part is not the code, but the documentation ;). Let me know if that section makes sense.

@guillaumebriday
Copy link
Member

guillaumebriday commented Sep 11, 2019

It makes sens ! The google's instructions are not very clear imo.

I think this line should be removed.

@clupprich
Copy link
Contributor Author

🤦‍♂

@gauravtiwari gauravtiwari merged commit 92b75e4 into rails:master Sep 18, 2019
@gauravtiwari
Copy link
Member

Thanks @clupprich

@clupprich
Copy link
Contributor Author

🎉 Thanks @guillaumebriday and @gauravtiwari

@clupprich clupprich deleted the brotli-support branch September 18, 2019 10:27
@guillaumebriday
Copy link
Member

🎉

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.

3 participants