Skip to content

fix: minimal guide for static assets #13

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

Closed
wants to merge 4 commits into from
Closed

fix: minimal guide for static assets #13

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
266f677
fix: minimal guide for static assets
wtrocki Oct 7, 2020
dbf2352
fix: migrate to proper format
wtrocki Oct 9, 2020
65e6ef5
fix: move static assets documentation into right directory
wtrocki Nov 10, 2020
5d8825c
Update docs/functional-components/static-assets.md
wtrocki Nov 11, 2020
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
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ with only a subset of sections having recommendations):
* Transactions_handling

* Development
* Static Assets
* [Static Assets](./docs/functional-components/static-assets.md)
* Keeping up to date
* Code Quality
* [Code Consistency](./docs/development/code-consistency.md)
Expand Down
26 changes: 26 additions & 0 deletions docs/functional-components/static-assets.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
## Static assets

## Recommended packages

* [express-static](https://expressjs.com/en/starter/static-files.html).
Express static is part of the express.js package that allows developers to expose static middlewares.

## Guidance

For serving static resources we recomend using `express.static` middleware as it has been widely used and tested in production.
When using express.static() method, we can server static resources directly by specifying the folder name where we have stored our static resources.

Documentation for the middleware can be found in:
https://expressjs.com/en/starter/static-files.html
Copy link
Contributor

@roastlechon roastlechon Nov 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Additional guidance for why or why not to serve assets out of application

Serving assets out of application

  • Makes application docker images self serving of dependencies (no need to deploy/sync assets to S3 to have application running)
    • No need for dedicate step to sync assets (less devops headache)
    • No need for application code to hard code domain path for dedicated static assets domain
    • Allows relative path for assets for ease of portability
  • DeployReviews could go straight to production (since docker images are self serving, these are not dependent on external resources like S3)
  • Misconception: Assets do not bloat the docker image as they are small in comparison to poorly written docker images which do not appropriately remove unused code. Eg docker node-alpine image vs node.)
  • Assets will already be fronted by Akamai CDN and incur less of a hit to origin.
  • Sabotages h2 push (not able to utilize h2 due to separate domain)

Serving assets out of S3

  • Has benefit of not incurring application origin hit cost
    • Although static assets would still be fronted by Akamai CDN
  • Has issues syncing assets from environment to environment
  • COS migration is harder as application is dependent on specific buckets/domain names for where assets will live.
    • Deploying to specific COS buckets means specific secret and access keys per bucket
    • Deploying to specific COS buckets also means associated akamai rules per bucket

Copy link
Member Author

Choose a reason for hiding this comment

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

@roastlechon knowing how much effort you have put I would not feel good to apply this change myself. I think it will be the best if you take this over and apply this change directly. Simply because this is exactly what we talked about and I would not say it better.

Copy link
Contributor

Choose a reason for hiding this comment

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

i can pull your branch and create new PR if that would be okay by you. I can reference existing comments/feedback in the new PR, while you focus more on the other PRs (graphql)

Copy link
Member Author

@wtrocki wtrocki Nov 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Anything you will like. Really I haven't done much and amount of knowledge you have brought here is amazing. Feel free to create new PR, close this one - anything that will minimize hassle on your side


### Caching

The static middleware does server-side caching.
It lets you do two methods of client-side caching: ETag and Max-Age
Copy link
Contributor

Choose a reason for hiding this comment

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

Add reference to https://developer.mozilla.org/en-US/docs/Web/HTTP/Caching

Copy link
Member Author

Choose a reason for hiding this comment

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

Cover only max-age - we need to change default argument.
Reference spec: http://expressjs.com/en/resources/middleware/serve-static.html

If you configured your static middleware to use client side caching please make sure that
Copy link
Member Author

Choose a reason for hiding this comment

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

We need that differentiated.

Copy link
Member Author

@wtrocki wtrocki Nov 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Mention that ETag is not required. CacheControl only.
Disable ETAG.

ETAG - difference between usages. We need to dig deeper why not reliable.

Copy link
Contributor

Choose a reason for hiding this comment

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

https://stackoverflow.com/a/58122775/12587417

Copy link
Contributor

Choose a reason for hiding this comment

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

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/ETag

Copy link
Member Author

Choose a reason for hiding this comment

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

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/ETag

Copy link
Contributor

Choose a reason for hiding this comment

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

https://www.freecodecamp.org/news/an-in-depth-introduction-to-http-caching-cache-control-vary/ reference for images and decision/sequence diagrams

every modification in your code will be creating resources with different file name.

Copy link
Contributor

@roastlechon roastlechon Nov 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

TODO for me to add section about middlelayers to server (nginx, apache, varnish, cdns, etc)

When using bundlers you will explicitly need to generate different file names every time content changes.
Example for webpack (most popular bundler) can be found here:
Copy link
Contributor

Choose a reason for hiding this comment

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

Could mention if hash is used in filename, max age could be as long as a year.


https://webpack.js.org/guides/caching