search

bbc / simorgh

The BBC's Open Source Web Application. Contributions welcome! Used on some of our biggest websites, e.g.
https://www.bbc.com/thai
Other
1.38k stars 215 forks source link

Add `cache-control` header to Next.js pages #11684

Closed amoore108 closed 2 months ago

amoore108 commented 2 months ago

Overall changes

Sets the cache-control header on Next.js pages to ensure the CDN serves cached copies of the page to users where applicable.

Testing

  1. List the steps used to test this PR.

Helpful Links

Add Links to useful resources related to this PR if applicable.

Coding Standards

Repository use guidelines

amoore108 commented 2 months ago

Probably worth having a unit test to check the header is set correctly on responses

Would that be in the form of an integration test? Or just a unit test to check that setHeaders is called with the correct cache-control values.

HarveyPeachey commented 2 months ago

Probably worth having a unit test to check the header is set correctly on responses

Would that be in the form of an integration test? Or just a unit test to check that setHeaders is called with the correct cache-control values.

I guess just a check it's called with the correct cache values will do. Integration might be overkill, I guess at that point you're kind of testing the framework, not sure what the best testing strat is really

amoore108 commented 2 months ago

Probably worth having a unit test to check the header is set correctly on responses

Would that be in the form of an integration test? Or just a unit test to check that setHeaders is called with the correct cache-control values.

I guess just a check it's called with the correct cache values will do. Integration might be overkill, I guess at that point you're kind of testing the framework, not sure what the best testing strat is really

I'll add a unit test for it, but worth thinking about how we test getServerSideProps and the responses in general. I think we use supertest for the Express app, but not sure how well it would integrate with Next, as as you say you're kind of testing implementation details at that point.