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 optional cache headers for components #325

Closed
i-b-dimitrov opened this issue Nov 28, 2016 · 7 comments
Closed

Add optional cache headers for components #325

i-b-dimitrov opened this issue Nov 28, 2016 · 7 comments

Comments

@i-b-dimitrov
Copy link
Contributor

We are thinking of adding some optional cache headers per component. For instance if you ask for http://..registry../hello-world/1.0.0 you would get some cache-control headers and benefit from CDNs or browser caches. We could use the component's package.json file as a carrier of those additional headers.

For example for hello-world component the package.json file could look like:

{
  "name": "hello-world",
  "description": "",
  "version": "1.0.0",
  "repository": "",
  "oc": {
    "files": {
      "data": "server.js",
      "template": {
        "src": "template.html",
        "type": "handlebars"
      }
    }
  },
  "headers": {
    "Cache-Control": "public max-age=3600",
    "Custom-Header": "custom-header-value"
  }
}

Then this headers section could be merged with the actual HTTP response headers but could also be added in the component's metadata:

(1) When rendered:

{
  "type": "oc-component",
  "version": "1.0.0",
  "requestVersion": "1.0.0",
  "name": "hello-world",
  "renderMode": "rendered",
  "headers": {
    "Cache-Control": "public max-age=3600",
    "Custom-Header": "custom-header-value"
  },
  "href": "http://registry_url/hello-world/1.0.0",
  "html": "...component-html..."
}

and (2) When unrendered:

{
  "type": "oc-component",
  "version": "1.0.0",
  "requestVersion": "1.0.0",
  "name": "hello-world",
  "renderMode": "unrendered",
  "headers": {
    "Cache-Control": "public max-age=3600",
    "Custom-Header": "custom-header-value"
  },
  "href": "http://registry_url/hello-world/1.0.0",
  "data": {
    
  },
  "template": {
    "src": "http://registry_url/hello-world/1.0.0/static/template.js",
    "type": "handlebars",
    "key": "737b5c8ac8536a42972facd7f2bbfc945326df42"
  }
}

However this is going to work for single components only. In case of bulk requests we could still return the headers entry in the components' metadata for use in the client if necessary but there is no point of setting cache headers in the HTTP response (as the request is POST and it normally will not be cached by the intermediate proxies).

What are your thoughts on that?

@matteofigus
Copy link
Member

Hi @ivan-dimitrov-skyscanner I would like for components to have the ability of setting response headers. I was talking about this feature with @seif and @antwhite too.

I would add @ferewuz to the conversation here. I think he raised in another PR a couple of interesting points for not having the headers set as a configuration (in the package.json as you suggest) - but instead to have them dynamically set inside the server.js. I guess a way would be to have something like

module.exports = function(context, callback){
  context.setHeader('key', 'value');
  ...
};

The advantage is more flexibility. For example in case something goes wrong, you may still want to return a failover response without caching headers, so that in the following request retries all the stuff and sends the caching header only if the response is 100% correct.

@i-b-dimitrov
Copy link
Contributor Author

Hi @matteofigus,

Agree with you on the point with the flexibility. But can't we have also a simplified approach allowing us to specify a general cacheability in a constant way. For example if you delivering some news page and you know the news will update once a day. So my proposal is to allow specifying the headers in both ways as I would say perhaps 95% of the components will not need complex logic for defining the cache. For the other 5% we will allow for programmatic approach as you proposed (and if defined, it would have a priority over the static headers).
Also some default configurable cache headers should take place. For example currently it is "no cache headers" but in some more aggressive environment the registry may say "everything should be cached forever unless other is specified by the component"

@seif
Copy link
Member

seif commented Nov 29, 2016

While I agree with @ivan-dimitrov-skyscanner that having some headers set in package.json is simpler, I think giving component authors the flexibility covers more bases and requirements initially. i.e. if we are going to start with something, let's start with programmatic and look into adding the static headers later.

@vanclevstik
Copy link

While I did agree with @ivan-dimitrov-skyscanner that there is a place for static headers, I do agree that dynamic ones make more sense and are much more flexible.
It still allows components to set them "statically" in their own config and always return them as part of response without any specific logic.

@i-b-dimitrov
Copy link
Contributor Author

OK, then can I assume that we have an agreement upon the dynamic version?
I'm happy to start working on this :-)

@matteofigus
Copy link
Member

Yes, thanks @ivan-dimitrov-skyscanner

i-b-dimitrov added a commit to i-b-dimitrov/oc that referenced this issue Dec 1, 2016
* Added a function setHeader(name, value) to the context object that we pass to the component data function.

* Subsequently these headers (if added) will be set in the response data in the returned component JSON as well as response headers (component.js)

* Added a mock component (response-headers-component) and a test to cover the new functionality
matteofigus pushed a commit that referenced this issue Dec 2, 2016
* Added a function setHeader(name, value) to the context object that we pass to the component data function.

* Subsequently these headers (if added) will be set in the response data in the returned component JSON as well as response headers (component.js)

* Added a mock component (response-headers-component) and a test to cover the new functionality
@matteofigus
Copy link
Member

Fixed by #326

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

No branches or pull requests

4 participants