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.

Sign up for GitHub

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

Jump to bottom

Patterns deployment with hashes #1089

Closed
wants to merge 2 commits into from
Closed

Patterns deployment with hashes #1089

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1a73573
Allow patter library to be served in dev from build artifcats
acelaya Jun 9, 2023
528e3a1
Add caching options to nginx config file
acelaya Jun 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,16 @@
FROM node:19.8.1-alpine as builder
FROM node:20.2.0-alpine as builder

COPY . /frontend-shared
RUN cd /frontend-shared && \
yarn install --frozen-lockfile && \
yarn build-pattern-lib


FROM nginx:1.24.0-alpine
FROM nginx:1.25.0-alpine

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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is no longer needed, because the index file is now built and placed in the build directory, so the step above includes it.

COPY ./images /usr/share/nginx/html/images
COPY conf/nginx.conf /etc/nginx/conf.d/default.conf

Expand Down
15 changes: 14 additions & 1 deletion conf/nginx.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,17 @@ server {
return 200 'Success!';
}

# JS and CSS files can be saved for a year, as they include a hash.
location ~* \.(?:css|js)$ {
expires 1y;
add_header Cache-Control "public";
}
# TODO Decide on a sensible cache period for images
location ~* \.(?:jpg|gif|png|ico|svg)$ {
expires 1d;
add_header Cache-Control "public";
}

# When requesting static paths with an extension, try them, and return a 404 if not found
location ~* .+\..+ {
try_files $uri $uri/ =404;
Expand All @@ -18,6 +29,8 @@ server {
# When requesting a path without extension, try it, and return the index if not found
# This allows for client-side route handling
location / {
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;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}
31 changes: 26 additions & 5 deletions gulpfile.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,20 +1,41 @@
import { buildCSS, runTests, watchJS } from '@hypothesis/frontend-build';
import {
buildCSS,
buildJS,
runTests,
watchJS,
} from '@hypothesis/frontend-build';
import gulp from 'gulp';

import { buildPatternLibraryIndex } from './scripts/build-pattern-library-index.js';
import { servePatternLibrary } from './scripts/serve-pattern-library.js';
import tailwindConfig from './tailwind.config.js';

gulp.task('serve-pattern-library', () => {
servePatternLibrary();
});

// The following tasks bundle assets for the pattern library for use locally
// during development. Bundled JS and CSS are not published with the package.

gulp.task('build-pattern-library-index', async () =>
buildPatternLibraryIndex()
);

gulp.task('bundle-css', () =>
buildCSS(['./styles/pattern-library.scss'], { tailwindConfig })
);

gulp.task('bundle-js', () => buildJS('./rollup.config.js'));

gulp.task(
'build-pattern-library',
gulp.series(
gulp.parallel('bundle-js', 'bundle-css'),
'build-pattern-library-index'
)
);

gulp.task(
'serve-pattern-library',
gulp.series('build-pattern-library', () => servePatternLibrary())
);

gulp.task(
'watch-css',
gulp.series('bundle-css', () =>
Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@
"scripts": {
"build-lib": "babel src/ --out-dir lib/ --extensions '.js,.ts,.tsx' --source-maps --ignore '**/test' --ignore '**/karma.config.js'",
"build": "yarn build-lib && tsc --build src/tsconfig.json",
"build-pattern-lib": "yarn gulp bundle-css && yarn rollup -c rollup.config.js",
"build-pattern-lib": "yarn gulp build-pattern-library",
"typecheck": "tsc --build src/tsconfig.json",
"lint": "eslint --cache .",
"checkformatting": "prettier --cache --check '**/*.{js,scss,ts,tsx,md}'",
Expand Down
25 changes: 25 additions & 0 deletions scripts/build-pattern-library-index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
import { generateManifest } from '@hypothesis/frontend-build';
import fs from 'fs';

const templateMap = [
['{{ css_bundle }}', 'styles/pattern-library.css'],
['{{ js_bundle }}', 'scripts/pattern-library.bundle.js'],
];

export async function buildPatternLibraryIndex() {
const manifest = await generateManifest({
pattern: 'build/{scripts,styles}/pattern-library*.{css,js}',
});
const templateContent = fs
.readFileSync('templates/index.template.html')
.toString();

const processedTemplateContent = templateMap.reduce(
(content, [pattern, fileName]) => {
return content.replace(pattern, manifest[fileName] ?? pattern);
},
templateContent
);

fs.writeFileSync('build/index.html', processedTemplateContent);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The two most important things IMO are:

  1. It is obvious what is/is not a variable in the template
  2. 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

}
2 changes: 1 addition & 1 deletion scripts/serve-pattern-library.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ export function servePatternLibrary(port = 4001) {

// For any other path, serve the index.html file to allow client-side routing
app.get('/:path?', (req, res) => {
res.sendFile(path.join(dirname, '../templates/index.html'));
res.sendFile(path.join(dirname, '../build/index.html'));
});

app.listen(port, () => {
Expand Down
4 changes: 2 additions & 2 deletions templates/index.html → templates/index.template.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 }}">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

</head>
<body>
<div id="app"></div>
<script type="module" src="/scripts/pattern-library.bundle.js"></script>
<script type="module" src="/{{ js_bundle }}"></script>
</body>
</html>