Patterns deployment with hashes #1089
Conversation
|
|
||
| RUN rm -r /usr/share/nginx/html && rm /etc/nginx/conf.d/default.conf | ||
| # Copy pattern library static assets, and put them in nginx document root folder | ||
| COPY --from=builder /frontend-shared/build /usr/share/nginx/html | ||
| COPY ./templates/index.html /usr/share/nginx/html/index.html |
There was a problem hiding this comment.
This is no longer needed, because the index file is now built and placed in the build directory, so the step above includes it.
| try_files $uri $uri/ /index.html$is_args$args; | ||
| # The index file references CSS and JS bundles, so we need to make sure it's always up to date | ||
| expires -1; | ||
| try_files $uri $uri/ /index.html; |
There was a problem hiding this comment.
I have removed $is_args$args, because those actually make no sense when loading a static file. They are meant to be used in fast-cgi contexts, like with PHP and such, where there might be some server-side logic run before nginx returns the result.
| templateContent | ||
| ); | ||
|
|
||
| fs.writeFileSync('build/index.html', processedTemplateContent); |
There was a problem hiding this comment.
I considered introducing mustache back, but the use case is so simple that it felt unnecessary, but I'm open to do it. No strong opinion here.
There was a problem hiding this comment.
The two most important things IMO are:
- It is obvious what is/is not a variable in the template
- Appropriate escaping is applied
I think we can get away with fake templates for now, although it does mean that someone will have to know to change this if they decide to add other dynamic content in future.
There was a problem hiding this comment.
although it does mean that someone will have to know to change this if they decide to add other dynamic content in future.
The main challenge here is mapping the manifest entries with their corresponding template variable, in a way that we don't couple the logic with the contents of the manifest itself (as it is above)
However, this is true even if we use a proper templating system. For example, with mustache this could look something like this:
const templateMap = [
['css_bundle', 'styles/pattern-library.css'],
['js_bundle', 'scripts/pattern-library.bundle.js'],
];
const manifest = await generateManifest({
pattern: 'build/{scripts,styles}/pattern-library*.{css,js}',
});
const templateContent = fs
.readFileSync('templates/index.template.html')
.toString();
const viewParams = templateMap.reduce(
(view, [pattern, fileName]) => {
view[pattern] = manifest[fileName] ?? pattern
return view;
},
{}
);
const processedTemplateContent = Mustache.render(templateContent, viewParams);
fs.writeFileSync('build/index.html', processedTemplateContent);The obvious benefit of a templating system is that the replacement of variables is less fragile (extra/missing spaces and such).
| @@ -4,10 +4,10 @@ | |||
| <meta charset="UTF-8"> | |||
| <meta name="viewport" content="width=device-width, initial-scale=1"> | |||
| <title>UI component pattern library</title> | |||
| <link rel="stylesheet" href="/styles/pattern-library.css"> | |||
| <link rel="stylesheet" href="/{{ css_bundle }}"> | |||
There was a problem hiding this comment.
I suggest the whole path should be a variable, including the leading /. If the file was moved for example, that part might need to change.
|
If using query-string cache busters like h-assets, there are a couple of issues to be aware of:
In h-assets we mitigate issue (1) and also other potential problems with caching by not setting any custom cache headers instead just leaving Cloudflare to use its default value of 30 minutes. This "solution" also limits how long issue (2) can last for. I would be inclined to do the same here. The main function of the hashes is to function as a cache buster after deployment. In the Hypothesis client, the asset URLs do have query-string cache busters, but that is just an accident of implementation as asset URLs include the client version. We also keep the assets for every client release around indefinitely and we don't make braking API changes to stable Hypothesis APIs. These effectively avoid both issues (1) and (2). |
|
Ugh, you are right. Those two points are a problem. I implemented this with the wrong assumption that the hash in the query string would be equivalent to having it in the file name, but it clearly isn't. I think I'm going to park this for now, as I don't know how much time is reasonable to spend on this. |
This PR introduces a number of changes with the intention to fix how Cloudflare caches the pattern library assets, trying to be as optimal as possible, and affecting building as little as possible.
index.htmlentry point used both during dev and in production, is now a template file, where the references to JS and CSS bundles is set dynamically.generateManifestfunction fromfrontend-build, which brings the ability to have content hashes in URLs, without affecting the actual building of those assets.nginx.conffile used in production now includes instructions to make sure properCache-Controlheaders are exposed to Cloudflare. Thos instructions imply that:Testing steps
There are a couple of things that need to be checked for regressions, and new logic to be tested.
make devdocker build . -t hypothesis_frontend_shared.docker run --rm -p 89:5001 hypothesis_frontend_sharedCache-Control: publicandCache-Control: max-age=31536000.