implement media server in MapeoManager

[achou11]

Closes #329

Notable changes:

• MapeoManager
  • adds mediaServerOpts constructor opt for configuring fastify server instance
  • instantiates fastify server and registers blob server plugin (abstracted to MediaServer)
  • adds async [kClose]() method, which is needed for tests in order to properly close the fastify server (made into public close method)
• MapeoProject
  • removes blob server instantiation
  • adds constructor opt getMediaBaseUrl(mediaType)
  • exposes BlobStore instance as symbol property (used for fastify plugin registration)
• BlobApi
  • adds constructor opt getMediaBaseUrl() and updates implementation of getUrl()
  • removes exposure ofBlobStore instance as public field
• Blob server fastify plugin
  • removes createBlobServer() implementation entirely since it was just a convenience around combining fastify with the plugin
  • creates new directory src/fastify-plugins/ to hold plugins (icons server plugin should go here eventually)
  • updates route param matching for project ids since they are public ids i.e. encoded as z-base-32 52-character string

Added a couple of new tests for the BlobApi.getUrl() changes but aside from that, mostly of the changes in the tests are cosmetic or to update given changed signatures.

[EDIT 1]

• After initial feedback:

  • introduces MediaServer class
  • introduces start() and stop() methods on MapeoManager class

[gmaclennan]

Good work on this. I think the remaining issue is around starting and stopping the media server. Because we are avoiding listening on a port when an app is in the background, the backend implementation on mobile will need to do something like:

rn_bridge.app.on('pause', (pauseLock) => {
  server.close( () => pauseLock.release())
})

I suggest just moving mediaserver into its own class, with start() and stop() methods. In the MapeoManager constructor we can do:

this.#mediaServer = new MediaServer()
this.#mediaServer.start()

Although I half think maybe to not start in the constructor and instead require you to call mapeoManager.start(port?) first? That's more "proper" but less intuitive from a dev perspective (although working with any server is generally the same - creating an instance does not auto-start)

[achou11]

Although I half think maybe to not start in the constructor and instead require you to call mapeoManager.start(port?) first?

yeah i agree. think it's intuitive enough. should the method name be more specific to highlight that it's just the mediaServer that's starting? or will there be other things that this start method will handle later on? EDIT: saw that you mentioned the dns-sd stuff will be added here too

[achou11]

apparently replacing the fastify instance is the only way to re-start the server? maybe there's something i missed in my search but thought there would be something built-in for this (or a plugin)

[gmaclennan]

Best place to search is our own code 🙂 https://github.com/digidem/mapeo-mobile/blob/develop/src/backend/upgrade-manager/upgrade-server.js#L201-L228

[achou11]

running into issues attempting to port that implementation.

I removed the conditional check here, and in #startServer() I have:

if (this.#fastifyStarted) {
  const { server } = this.#fastify
  await promisify(server.listen.bind(server))(this.#port, this.#host)
} else {
  await this.#fastify.listen({ host: this.#host, port: this.#port })
  this.fastifyStarted = true
}

running into a type issue with the promisify and after a brief search, my guess is that it's maybe related to @types/node. even so, running through a lifecycle test, I still get a fastify error about the server already listening (FastifyError [Error]: Fastify is already listening). Guessing there's still something I'm missing...

[achou11]

i was setting this.fastifyStarted instead of this.#fastifyStarted... hours wasted on this 🙃

[achou11]

re: the promisify type error, seems like it's complaining for a potentially legitimate reason?

seems like the callback param of server.listen() doesn't follow the cb: (err, val) => void convention. turns out that the callback is only ever called on success, so you'll never get a rejection when wrapping with promisify.

https://nodejs.org/docs/latest-v16.x/api/net.html#serverlisten:

This function is asynchronous. When the server starts listening, the 'listening' event will be emitted. The last parameter callback will be added as a listener for the 'listening' event.

Seems like doing something along the lines of this would be work though: nodejs/node#21482 (comment)

[gmaclennan]

Looks good, but a few bugs - I added a test to catch them and fixed some of them.
Remaining bug is due to a bug in the blob-api hashing - I'd been meaning to circle back to that because I suspected it wasn't right. You can't pipe through a hash stream because the output is the hash: we were writing the hash to the blob store rather than the file contents. Also see my comment about starting and stopping fastify.

[gmaclennan]

This fixes the e2e media-server test, but it breaks the blob-api test because the hash is incorrect.