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

Enable disabling cache-control headers #3044

Merged
merged 1 commit into from Mar 11, 2016
Merged

Enable disabling cache-control headers #3044

merged 1 commit into from Mar 11, 2016

Conversation

@csabapalfi
Copy link
Contributor

@csabapalfi csabapalfi commented Feb 3, 2016

Sometimes you just don't want Cache-Control headers at all.

One example is old IE versions not loading fonts with a Cache-Control header.

@csabapalfi
Copy link
Contributor Author

@csabapalfi csabapalfi commented Feb 24, 2016

Is anyone looking at hapi PRs at all? @hueniverse?

@mtharrison
Copy link
Contributor

@mtharrison mtharrison commented Feb 24, 2016

It would feel more consistent with existing API IMO if you could just set cache to false instead of the disabled option. Also I think you should add tests.

@csabapalfi
Copy link
Contributor Author

@csabapalfi csabapalfi commented Feb 24, 2016

Thanks for the feedback, will simplify it and add tests!

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Feb 24, 2016

I am looking at PRs but this is not a high priority. Since I am not getting paid by anyone to do this, my priorities are pretty limited to security, major bugs, and stuff I need myself. Features like that tend to wait for a release for one of the above reasons (since I'm already spending time preparing it).

That said, most things get merged pretty fast. This one is just very low on my list and has very little interest from other people.

@csabapalfi
Copy link
Contributor Author

@csabapalfi csabapalfi commented Feb 24, 2016

Understood and makes sense. Maybe just tagging it (even if the tag says low-priority) would let everyone know about the situation. Actually not touching the PR at all is probably even clearer. Thanks.

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Mar 9, 2016

@csabapalfi please make the changes suggested by @mtharrison

@csabapalfi
Copy link
Contributor Author

@csabapalfi csabapalfi commented Mar 9, 2016

@hueniverse Done. Thanks for looking into this!

statuses: Joi.array().items(Joi.number().integer().min(200)).min(1)
}),
Joi.boolean().valid(false)
),
Copy link
Contributor

@hueniverse hueniverse Mar 10, 2016

Instead just add .allow(false) to the existing object rule.

Copy link
Contributor Author

@csabapalfi csabapalfi Mar 10, 2016

Thanks, that's much simpler. Done.

@AdriVanHoudt
Copy link
Contributor

@AdriVanHoudt AdriVanHoudt commented Mar 10, 2016

Can I suggest updating the docs as well stating this can be set to false? https://github.com/hapijs/hapi/blob/master/API.md#route-options

@csabapalfi
Copy link
Contributor Author

@csabapalfi csabapalfi commented Mar 10, 2016

@AdriVanHoudt 👍 Good point. Updated the docs.

Signed-off-by: Csaba Palfi <csaba@palfi.me>
@AdriVanHoudt
Copy link
Contributor

@AdriVanHoudt AdriVanHoudt commented Mar 10, 2016

🚀

@hueniverse hueniverse self-assigned this Mar 11, 2016
@hueniverse hueniverse added this to the 13.1.1 milestone Mar 11, 2016
hueniverse added a commit that referenced this issue Mar 11, 2016
Enable disabling cache-control headers
@hueniverse hueniverse merged commit 687a378 into hapijs:master Mar 11, 2016
1 check passed
hueniverse added a commit that referenced this issue Mar 11, 2016
@lock
Copy link

@lock lock bot commented Jan 9, 2020

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants