-
-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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: make compression middleware customizable #3863
feat: make compression middleware customizable #3863
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #3863 +/- ##
=========================================
- Coverage 97.8% 97.48% -0.32%
=========================================
Files 18 18
Lines 1183 1193 +10
Branches 325 328 +3
=========================================
+ Hits 1157 1163 +6
- Misses 25 28 +3
- Partials 1 2 +1
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about using hook like render:before or move render.gzip to a common render.commpressor for denfining custom compression lib.
@clarkdo You mean like:
|
Yeah, how do you think? |
Sounds great and will make things definitely easier! I'm on it! |
After thinking a few minutes, |
3390308
to
09b5383
Compare
09b5383
to
3a15940
Compare
3a15940
to
d391655
Compare
If everything fits I'd write the docs and then this PR is good to go |
lib/common/options.js
Outdated
@@ -4,6 +4,7 @@ import fs from 'fs' | |||
import _ from 'lodash' | |||
import consola from 'consola' | |||
|
|||
import compressionMiddleware from 'compression' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m not sure if put compression here is proper, maybe we can make it lazy required and make options.js only change config, @pi0 do you have any idea.
BTW name can be compression?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've renamed it 👌
Could you explain how you mean lazy loading it?
In the end, it's "just" the server bundle anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
require or nuxt.requireModule inside renderer so that we can support sth like: compressor: ‘custom-lib’ or compressor: { name: ‘compression’, options: {}}. Maybe I thought too much, current pr is good for me. Let’s ask @pi0 review and merge.
Thanks, @manniL @clarkdo. Changes are quite good. But I think we should move compression initialization (retrieve) logic into |
fe50958
to
134a26f
Compare
2046e78
to
4d69f11
Compare
Fix legacy gzip warning. Thanks to @liam-potter for pointing that out! Related PR: #3863 ## Types of changes - [x] Bug fix (non-breaking change which fixes an issue) ## Checklist: - [x] All new and existing tests passed.
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Let user customize compression middleware
Types of changes
Description
The user can now set up his own compression middleware on
render.customCompressionMiddleware
which will then be used instead ofcompression
.Could help withLikely resolves #3117 👍 (have to test it on HTTPS first)Checklist: