Implement new library to retrieve static assets URL

[philippemiguet]

This is ready for review. It is being used in this PR: https://github.com/bufferapp/buffer-login/pull/47.

This first version will work well for SSR. I was thinking we could work on a client-side React version during the next hack-week :)

[djfarrelly]

We may want "stable" or 10 also since that's the current version in development, not important, but it crossed my mind when I saw this :)

[philippemiguet]

@djfarrelly shall we use then node or lts/* instead of 8? https://docs.travis-ci.com/user/languages/javascript-with-nodejs/#specifying-nodejs-versions

[djfarrelly]

@philippemiguet I think just lts/* should work which is now node 10 and we should be using only LTS releases for our production services 😄 . If this ends up using language features that only exist in node 10 and we need to polyfill for node 8 or below we can add 8. I think this repo should use pretty standard/stable language features so running the tests against 10 😄

[djfarrelly]

Hey @philippemiguet, in reviewing this, I am re-thinking the original approach that I took with the code for the flexible use case across different types of projects. I think you can go ahead with the beta branch for on buffer-login perhaps, but I think we should refactor a bit before making this a stable release and encouraging others to use it!

Sorry if this is a ton of thoughts!! 😄

[djfarrelly]

This error message may change in future versions of node, should we make this assertion check against err.code === 'ENOENT'?

[djfarrelly]

Should we add a default value here to match what normally gets generated from buffer-static-upload ? 🤔

[djfarrelly]

I'm curious, why does staticUrl use multiple arguments when getStaticUrl uses an options object/keyword args? Could they both just follow the same staticUrl(filename, cacheBust) pattern for simplicity?

[philippemiguet]

Good one! If we had to pick one pattern, I would use the object/keywords args since it is the one we are using everywhere else. It feels better for maintainability IMO: no param ordering when adding / removing one for instance, or easier to construct params to be passed.

I think I switched to multiple args for staticUrl to reduce the "footprint" and have a less verbose function to use: staticUrl('/buffer.png', true) instead of staticUrl({filename: '/buffer.png', cacheBust : true}).

Happy to go with one or the other :)

[djfarrelly]

Thanks @philippemiguet, yeah I was thinking of using one pattern for simplicity sake, but with that thinking, I'm cool with the pattern you already have. We also could support both by checking if the first arg is an object or string, but that could be a future optimization of the library :)

[djfarrelly]

Since this is set via setStaticAssetVersions I don't know if it needs to be exposed as an option for the class. Same with staticAssetVersions, when would that be passed directly to the class constructor since it's not exposed outside of this file? What do you think?

[djfarrelly]

We could potentially make these arg names less verbose too since they're all within the static asset manager

[philippemiguet]

@djfarrelly great point! I don't recall well why I took this approach. I think what may have drove this is to make it useful for anyone as it is an open-source project. Let's simplify things 👍

[djfarrelly]

These tests are really useful to seeing the way this library could be used! I know I created the first version within buffer-marketing from what this is based on, but I'm not sure if my original approach is best for simple consumption in projects. Re-thinking the original approach I took, I think it could be a lot better and potentially have more clearer and simpler variable names and arguments as well as usage for different types of apps.

Bear with me while I re-think the original code I slapped together 😄 ....I'll try to write an examples of how someone might use this in their code to illustrate my thoughts:

const app = express()
app.get('/', someHandler)

// Version 1
async function start() {
  await initializeStaticAssetsManager({
    staticAssetVersionsFilename: process.env.NODE_ENV === 'production' ? 'staticAssets.json' : null,
  })
  app.listen(8080)
}
start()

// Version 2
initializeStaticAssetsManager({
  staticAssetVersionsFilename: process.env.NODE_ENV === 'production' ? 'staticAssets.json' : null,
}).then(() => app.listen(8080))

• We need to use an async function or use then() to ensure the app doesn't start accepting traffic before the manager is initialized. This could cause errors as staticUrl() would call a method on staticAssetsManager which would be undefined. Maybe this is OK, but if so, we should document in the readme or find a way to prevent someone from implementing this library without doing so.
• The line checking if the app's in production and sending the standard filename would likely be copied to every single application. I think we may be able to make this automatic, but configurable.

I'm trying to think of a way we could create a simpler way to use this? We also can consider how other projects are using it like here in New Publish which isn't maybe super ideal, but they are loading the file synchronously: https://github.com/bufferapp/buffer-publish/blob/0314d72edf0a11c3bd1df99855e4eb4abcd28044/packages/server/index.js#L39-L56

class StaticAssetManager {
  constructor({
     filename: 'staticAssets.json',
     useLocal: process.env.NODE_ENV === 'development', // default this, but allow for override
     path: '/', // the relative path where static assets are served during local dev
     localPort: null, // useful for teams using webpack dev server running outside the container (ex. publish)
     dir: 'public', // prefix for looking up static files in the staticAssets.json (the local directory prefix)
  }) {
    this.filename = filename
    this.useLocal = useLocal
    this.path = path
    this.localPort = localPort
    this.dir = dir
    this.versions = {}
  }

  load() {
    return new Promise((resolve, reject) => {
      fs.readFile(this.filename, (err, c) => {
        if (err) return reject(new Error(`StaticAssetsManger: Could not load static assets file '${this.filename}'`))
        resolve()
      })
    })
  }
}
let mgnr = null
module.exports.initStaticAssets = (options) => {
  mgnr =  new StaticAssetManager(options)
  if (mgnr.useLocal) return Promise.resolve()
  return mgnr.load()
}
module.exports.initStaticAssetsSync = (options) => {
  mgnr =  new StaticAssetManager(options)
  if (mgnr.useLocal) return
  mgnr.loadSync() // similar to above
}

// Standard usage
initStaticAssets()
  .then(() => app.listen(8080))
// Forcing local files even in prod
initStaticAssets({ useLocal: true }).then(...)
// Alt filename - don't need to check the NODE_ENV
initStaticAssets({ filename: 'assets.json' }).then(...)
// Force using the static assets all the time even in dev (useful for testing)
initStaticAssets({ useLocal: false }).then(...)
// Allow for synchronous usage like in buffer-publish
initStaticAssetsSync({ localPort: 8080 })
// Marketing site
initStaticAssets({ path: '/static', dir: 'public' }).then(...)

What do you think?

[philippemiguet]

Hey @djfarrelly great suggestions here! It's your turn to bear with me now 😂

---

Addition of the sync version

Great addition of the sync version in the package. I was initially thinking of documenting it to make sure people use it with async / await, but it seems I've forgotten myself to use async / await in buffer-login 🤦‍♂️That's a better way to provide both async and sync versions 👍

---

path: '/', // the relative path where static assets are served during local dev
localPort: null, // useful for teams using webpack dev server running outside the container (ex. publish)
dir: 'public', // prefix for looking up static files in the staticAssets.json (the local directory prefix)

I like the renaming of staticAssetDirLocal and staticAssetDirCDN to path and dir, and how they better reflect their role and how to be used.

However, I've found path and dir (previously, staticAssetDirLocal and staticAssetDirCDN) quite confusing to use. The following is true in both my and your version, that's regardless of the name :)

First use case: Buffer Login SSR w/ React
The need for those two variables is very tied to this first use case: loading assets from the public folder in React, and how we do things between express locally and our production CDN.

1. Our images assets are all in the /public folder, for instance /public/img/marketing-bg-tl.png
2. Locally, we don't need to specify /public in the assets URL because we told express to retrieve the assets from the public folder by default: here and here
3. When running pre-build.sh, the assets uploader will generate keys using the full path, so keys will look like public/img/marketing-bg-tl.png => CDN URL.
4. Then, in production, we need to append public to generate the right lookup key: that's where we need the dir variable.

Second use case: Webpack assets
If we want to make this library useful also for loading webpack assets, then dir we need to initialize the lib with dir: '/'. The reasons are:

1. Webpack build will generate vendor.js file in the main directory / not in /public
2. This current version will lookup for public/vendor.js.
3. When used with webpack dev server, we need to specify not only the port but the full URL https://local.buffer.com:8080/static/ to be used in development mode: getStaticURL('vendor.js') would return https://local.buffer.com:8080/static/vendor.js, and no need to create a local assets object.

This would work for now, while I still find the dir approach confusing.

Third use case: Both at once
Correct me if I'm wrong, but ultimately it seems we will need both at once. For instance, in buffer-login we will want to load the webpack builds and also load assets from the CDN in React. I think we will hit a wall with the configuration of path, localPort/URL and dir as outlined in the before, they will require different values.

• We may need to rethink a bit how we generate static URL from the lib using those 3 variables.
• At first, to move forward faster, we could provide two distinct functions: one for loading assets (images) in React, and the other for loading build files at the start of the app.
• We could also move the webpack generated files to the /public folder before they are being uploaded in the pre-build.sh file.

Other thought
Finally, this all work well on the server side, but if we want to use this library on the client side, we will need to think of where to keep the data (the list of key/value assets): will it be a store?

Actually do we have the intention of using it on the client side as well? I feel that would potentially be valuable since engineers wouldn't have to care anymore about uploading the assets themselves to the CDN.

---

If you are still there, what do you think of all the above? 😅 😄

[djfarrelly]

You've got some great points @philippemiguet.

re: awkward path/dir file paths:

• Maybe we can remove /public in both the key and CDN file path? If we wanted this we'd have to ship a breaking change to buffer-static-upload which may be a great idea!
• I think that might help remove the dir part of the config completely and remove the awkwardness you're sharing 😄

re: client side
We'll need a client side usage of this, but I see that mostly being for images since they'd be used in the React views. Since images don't have versioned/hashed file names we may just need the base directory. Here's a draft on how that could work:

// @bufferapp/static-assets/client.js
let staticFileBaseURL = '/static/' // default for local dev typically via express-static
export setBaseURL = (baseURL) => staticFileBaseURL = baseURL
export staticURL = (file) => `${staticFileBaseURL}${file}`

And in the application:

// app.js
import MyApp from './components/MyApp'
import { setBaseURL } from '@bufferapp/static-assets/client'

// this could be set by a global bootstrapped on the page
setBaseURL(window.staticAssetBaseURL) 
// -> we could even make this the default!

React.render(<MyApp/>, document.getElementById('#app'))

// ./components/MyApp.jsx
import { staticURL } from '@bufferapp/static-assets/client'

<div id="body></div>
<!-- in dev -->
<script> window.staticAssetBaseURL = '/static/' </script>
<!-- in prod -->
<script> window.staticAssetBaseURL = 'https://static.buffer.com/login/' </script>

We could even take this a step further and make some of this more automatic with a piece of express middleware that injects the staticAssetBaseURL into res.locals but that may be a bit overkill for now 😁

[djfarrelly]

I've decided to track any changes w/ the public dir prefix here: bufferapp/buffer-static-upload#15 cc @philippemiguet

[philippemiguet]

Hey @djfarrelly, great idea removing the public dir prefix from the generated keys, it will solve the issue. I left a comment directly in the issue you have opened!

re: local host and webpack dev server
The next version should remove the need for engineers to have to declare their own local assets to mock what the data would look like in production - example here from Publish repo.

With your idea of passing a localHostURL parameters, and useLocal, when using getStatic('vendor.js'), the lib would generate https://local.buffer.com:8080/static/vendor.js right away.

I wonder if webpack dev server can serve images as well. Curious what will happen when called for images then. Would https://local.buffer.com:8080/static/buffer.png work?

Link to issue: #3

re: client side
That's right I overlooked that, there is no hash so it is just a matter of appending the right base URL 👍 I do think that will help the engineers so they don't have to manually upload image files.

I think we also need to figure out how this part would work for the Buffer components shared library. The image files won't be in the root /static but in a relative path. Also, I think this team is uploading assets to CF and using the CF URLs in the code directly.

Link to issue: #4

[djfarrelly]

I wonder if webpack dev server can serve images as well. Curious what will happen when called for images then. Would https://local.buffer.com:8080/static/buffer.png work?

Good question - I think if webpack dev server is setup to be a proxy, it will serve the image on the express site.

For the other ones, I have added comments in the new issues :D Thanks for tracking all of these things in Github!

[philippemiguet]

Good question - I think if webpack dev server is setup to be a proxy, it will serve the image on the express site.

I'm adding a not to the #3 issue since it's related to passing a localHost URL to support webpack dev server, to figure out the config to make it work altogether.

[esclapes]

@djfarrelly wondering if we want to wait for Phil to move forward with this one. I am happy to work on this if necesary.

@ssvmvss is this blocking for you?

[djfarrelly]

@esclapes, I think we'll need to make some sort of change either in this code or in https://github.com/bufferapp/buffer-login/pull/47 to fix the potential race condition that could happen when loading static assets.

I'm also happy to dig into these changes. I don't think it will take me too long and I know you have a lot on your plate with the migration script!

[djfarrelly]

I just pushed a synchronous version of the same function to avoid potential race conditions in the short term!