Don't encode responses with Cache-Control: no-transform #53
Conversation
| @@ -135,6 +135,14 @@ function compression(options) { | |||
| return | |||
| } | |||
|
|
|||
| // Don't compress for Cache-Control: no-transform | |||
| // https://tools.ietf.org/html/rfc7234#section-5.2.1.6 | |||
| var cacheControl = res.getHeader('Cache-Control') || '' | |||
There was a problem hiding this comment.
Does this regular expression incorrectly match "foo-no-transform"?
There was a problem hiding this comment.
Hrm, you're right - I was specifically trying to catch that case!
I'll add some negative tests and improve this - probably split on ,, .trim() and string compare.
There was a problem hiding this comment.
I did have a look around for an existing lib, but didn't spot anything relevant.
There was a problem hiding this comment.
Yes, there is no existing lib I know of. I was going to write one and put it in the jshttp lib and use it for the implementation of the feature request, though.
There was a problem hiding this comment.
Well, I mean finish the one I started months ago, not start a new one from scratch 😊
glenjamin
force-pushed
the
no-transform
branch
from
70c5f8b
to
1659fd0
Compare
|
Not the tidiest implementation ever, but should be much more robust than the previous. Thoughts? |
|
Hi @glenjamin , sorry, I'm not ignoring you :) Was just swamped today, I'm sure you know how it goes :) Yes, seems good enough for now to accept and eventually swap out with a module sometime in the future (at least we'll have the feature today, which is what matter most ;) . I'll get this merged tonight and then release very soon! |
|
As a side note, I've been debating with myself if we should add an option to disable this feature, but IDK, haha. We don't have a switch to turn off Vary. I also debated with if this detection should be within the This comment was just internal dialog to give to you since you made the PR; not anything that is required to be acted upon :) |
|
I did wonder about making it configurable or putting into shouldCompress, but came to the same conclusion - if someone's set |
|
Sorry to bump, but is this good to merge? Let me know if some changes are still needed. |
|
Hey! Sorry, it is mostly OK, I was going to merge as part of getting this supported with Node.js 4. It also seems to take noticeable time to run this new code, with the split, the some, the trim, and creating a closure. This can easily be reduced to a single regular expression match, but I have not gotten to it, so if you can do that, awesome, otherwise I'll get to it :) so I technically am not waiting on you, haha. |
glenjamin
force-pushed
the
no-transform
branch
from
1659fd0
to
9d98c8b
Compare
|
Updated with a regex, do you have a benchmark suite or something to measure? I could probably poke about and see if there's a faster regex variant. |
|
Awesome, @glenjamin , that's it! I should have told you earlier, I'm sorry! As a maintainer, it's always hard to communicate well with people I have not had a history with yet :) Many times, people will just be turned off when there is continued problem noted, so I typically say the first couple, and then adjust after that for a positive experience |
|
No worries, I've been in your shoes a bunch of times :) |
|
I'm just getting some dependency updates in here with your change, just as an update. |
|
Any reasoning for choosing to place the check above the |
|
Published to npm as |
|
RE: Vary - not sure if I thought about it at the time, but I guess since there's no transforming going on so nothing varies? |
As discussed in #51 and webpack-contrib/webpack-hot-middleware#10